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

Reformat transfers to a format the hardware wallet already supports. #3418

Closed
wants to merge 2 commits into from

Conversation

murisi
Copy link
Contributor

@murisi murisi commented Jun 17, 2024

Describe your changes

While splitting the Transfer structure into 4 case makes sense, hardware wallet support would be simpler to achieve if we left the formatting of the test vectors unchanged. This PR is depended upon by Zondax/ledger-namada#47 .

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

Namada v0.39.0

Checklist before merging to draft

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

@murisi murisi marked this pull request as ready for review June 17, 2024 10:26
@murisi murisi requested a review from tzemanovic June 17, 2024 10:35
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 83 lines in your changes missing coverage. Please review.

Project coverage is 53.89%. Comparing base (879a326) to head (2f27850).
Report is 4 commits behind head on main.

Files Patch % Lines
crates/sdk/src/signing.rs 0.00% 83 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3418      +/-   ##
==========================================
- Coverage   53.92%   53.89%   -0.03%     
==========================================
  Files         317      317              
  Lines      107575   107615      +40     
==========================================
- Hits        58011    58001      -10     
- Misses      49564    49614      +50     

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

@murisi murisi requested a review from grarco June 17, 2024 15:19
Copy link
Member

@tzemanovic tzemanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for fixing this! Btw, if the rebuild of wasm_for_tests isn't necessary for this change, can we skip it? It makes merging easier as rerere is not very good with binary blobs

@Fraccaman Fraccaman mentioned this pull request Jun 21, 2024
@brentstone
Copy link
Collaborator

What's the status here? Is it an issue that this wasn't included in 0.40.0 release? It was never tagged as ready for draft

@brentstone brentstone mentioned this pull request Jul 6, 2024
@murisi
Copy link
Contributor Author

murisi commented Jul 8, 2024

What's the status here? Is it an issue that this wasn't included in 0.40.0 release? It was never tagged as ready for draft

This PR was made redundant by #3356 . Closing now, thanks.

@murisi murisi closed this Jul 8, 2024
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