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

Remove borsh schemas and the encoding spec #2682

Closed
wants to merge 5 commits into from
Closed

Conversation

sug0
Copy link
Collaborator

@sug0 sug0 commented Feb 21, 2024

Describe your changes

Borsh schemas were only depended upon by the namada_encoding_spec crate. Therefore, we can remove schemas, which were an unstable feature in borsh, anyway.

Indicate on which release or other PRs this topic is based on

v0.31.5

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@sug0
Copy link
Collaborator Author

sug0 commented Feb 21, 2024

might need to rebuild wasms for tests

@sug0 sug0 marked this pull request as ready for review February 22, 2024 10:40
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (c733be2) 53.38% compared to head (39b54dc) 53.76%.
Report is 2 commits behind head on main.

Files Patch % Lines
crates/tx/src/types.rs 72.72% 3 Missing ⚠️
.../protocol/transactions/ethereum_events/eth_msgs.rs 0.00% 1 Missing ⚠️
crates/proof_of_stake/src/types/mod.rs 75.00% 1 Missing ⚠️
crates/sdk/src/queries/vp/pos.rs 0.00% 1 Missing ⚠️
crates/vote_ext/src/lib.rs 50.00% 1 Missing ⚠️
crates/vote_ext/src/validator_set_update.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2682      +/-   ##
==========================================
+ Coverage   53.38%   53.76%   +0.38%     
==========================================
  Files         302      301       -1     
  Lines      103403   102602     -801     
==========================================
- Hits        55198    55162      -36     
+ Misses      48205    47440     -765     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tzemanovic added a commit that referenced this pull request Feb 26, 2024
* origin/tiago/rm-borsh-schemas:
  Changelog for #2682
  Stop building `namada_encoding_spec` in CI
  Remove borsh schema feat from Cargo manifest
  Remove borsh schema implementations
  Remove `namada_encoding_spec`
@murisi
Copy link
Collaborator

murisi commented Feb 27, 2024

Would keeping the BorshSchema's in Namada be harmful? I ask because these schemas would be extremely useful in describing our serialized formats to external collaborators like Zondax. Providing them with the (automatically derived) schemas would reduce the time we spend manually specifying serializations and simultaneously ease collaborators' process of writing and auditing parsers for our transactions in languages like C. The other more niche use case for these schemas is to automate the construction of parsers like in: #2732 .

@sug0
Copy link
Collaborator Author

sug0 commented Feb 27, 2024

we'll be keeping borsh schemas in Namada, as they might be handy in the future

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants