-
Notifications
You must be signed in to change notification settings - Fork 221
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
feat: generate script challenge on the ledger #6344
feat: generate script challenge on the ledger #6344
Conversation
Test Results (CI) 3 files 120 suites 36m 0s ⏱️ Results for commit d5c9a62. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files 11 suites 22m 50s ⏱️ For more details on these failures, see this check. Results for commit d5c9a62. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking nice, just a couple of comments
applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs
Show resolved
Hide resolved
applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs
Show resolved
Hide resolved
applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs
Show resolved
Hide resolved
applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, see if you can move the hashing to the hashing crate and that it still works.
I don't know if you want to leave the optimizations of the data types to a later pr?
We probably need to optimize all of them not just the script signature data
Make the script signature function a single payload function. We're passing less data, and the previous multi function format wasn't as clean as the metadata signature. Back down to one function call is more ideal.
d42c801
to
d5c9a62
Compare
Moved the hash domains into hashing/src/. This does require making that create a I looked at using rand_core instead of ledger's random impl, but it's not no_std friendly. I'll save the byte optimizations for another PR. |
@brianp, please see the comment about the u64 random^ |
Description
This generates the entire challenge for the script signature on the ledger.
Move signature signing functions into respective handler files.
Move hash domains into the hashing file.
Motivation and Context
The total payload size is smaller than before, and can now be done in a single message. With no batching needed I've removed the script signature context struct on the ledger side to reduce complexity, remove persisted state, and free up a bit of memory.
How Has This Been Tested?
Manually with new wallets
Breaking Changes