Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust Docs + refactor: protocols crates #845

Open
31 of 78 tasks
plebhash opened this issue Apr 12, 2024 · 7 comments
Open
31 of 78 tasks

Rust Docs + refactor: protocols crates #845

plebhash opened this issue Apr 12, 2024 · 7 comments
Assignees
Labels
documentation Improvements or additions to documentation refactor Implies refactoring code tracker Help us track a group of related issues

Comments

@plebhash
Copy link
Collaborator

plebhash commented Apr 12, 2024

We recently published our protocols crates to crates.io.
We need to make sure the APIs provided by those crates are properly documented under the Rust Best Practices.

This is also the perfect opportunity to map and address technical debt on those crates. As we go through the code and try to understand it, we take the chance to write down a list of ways the code could be improved. Things such as:

  • unintuitive, redundant, broken or suboptimal APIs
  • unsafe error handling (unwrap, panic!, expect)
  • typos, bad variable names
  • // todo comments, todo! macros, edge cases with unimplemented! or panic!

Task list

Rust Docs

Refactor Implementation

Dependencies graph

@plebhash plebhash added documentation Improvements or additions to documentation tracker Help us track a group of related issues labels Apr 12, 2024
@plebhash plebhash self-assigned this Apr 12, 2024
@pavlenex pavlenex added this to the Milestone 5 milestone Apr 12, 2024
@plebhash plebhash moved this from Todo 📝 to In Progress 🏗️ in SV2 Roadmap 🛣️ Jul 2, 2024
@plebhash plebhash modified the milestones: 1.2.0, 1.1.0 Jul 2, 2024
@rrybarczyk
Copy link
Collaborator

We can leverage the work done in #854 in this effort when we start listing the technical debt for each crate.

@jbesraa
Copy link
Contributor

jbesraa commented Sep 6, 2024

What do you think about merging all the v2/binary-sv2/** into a single v2/binary-sv2 for the following reason #1134 (comment) ?

@Fi3
Copy link
Collaborator

Fi3 commented Sep 6, 2024

I don't think that this is a good idea as we wouldn't be able to opt out serde anymore

@jbesraa
Copy link
Contributor

jbesraa commented Sep 6, 2024

Oh sorry maybe my comment was not clear. I was just suggesting to merge them in terms of the effort to document and refactor them, not merge the crates into a single one.

@plebhash
Copy link
Collaborator Author

plebhash commented Sep 16, 2024

What do you think about merging all the v2/binary-sv2/** into a single v2/binary-sv2 for the following reason #1134 (comment) ?

As discussed in last week's dev call (sep 10th 2024), binary-sv2/serde-sv2 is a candidate for deprecation. This crate is the only reason we maintain the with_serde feature flag, which is known to be causing some trouble on downstream projects. Therefore it is highly desirable that we eliminate the with_serde feature flag by getting rid of this crate.

So spending time documenting this crate would probably be wasted effort.

The only thing preventing us from deprecating it right now is the fact the MG uses it as a dependency.

@plebhash
Copy link
Collaborator Author

we are starting to write many ideas to our Technical Debt issues

we should allow ourselves room to brainstorm and propose ideas, this is the time to get creative

but when we start doing the actual refactoring PRs, it's of strategic importance that we cultivate a disciplined mindset around Goodhart's Law so we can make the right engineering tradeoffs and compromises

@Georges760
Copy link

maybe #1233 can fit in this issue ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation refactor Implies refactoring code tracker Help us track a group of related issues
Projects
Status: In Progress 🏗️
Development

Successfully merging a pull request may close this issue.

6 participants