Skip to content

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

Closed
murisi wants to merge 2 commits intomainfrom
murisi/reformat-transfer-testvectors
Closed

Reformat transfers to a format the hardware wallet already supports.#3418
murisi wants to merge 2 commits intomainfrom
murisi/reformat-transfer-testvectors

Conversation

@murisi
Copy link
Copy Markdown
Collaborator

@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
@codecov
Copy link
Copy Markdown

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
Copy Markdown
Collaborator

@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
Copy Markdown
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
Copy Markdown
Collaborator 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