Refactor wallet for public account changes#452
Conversation
jonesmarvin8
changed the base branch from
main
to
marvin/signature-bip340-fixes
This reverts commit 41f34f4.
This reverts commit d70ef50.
moudyellaz
left a comment
moudyellaz
left a comment
There was a problem hiding this comment.
lgtm, thanks for addressing my comments. Left 2 minor comments.
schouhy
left a comment
schouhy
left a comment
There was a problem hiding this comment.
Thanks for the PR. I'm finding hard to review as this looks like some minor adjustments for a bigger PR and I'd like to understand the bigger scope to see if I agree with the design choices.
| #[must_use] | ||
| pub fn from_list(signatures: &[Signature], pub_keys: &[PublicKey]) -> Self { | ||
| assert_eq!(signatures.len(), pub_keys.len()); | ||
|
|
||
| let signatures_and_public_keys = signatures | ||
| .iter() | ||
| .zip(pub_keys.iter()) | ||
| .map(|(sig, key)| (sig.clone(), key.clone())) | ||
| .collect(); | ||
|
|
||
| Self { | ||
| signatures_and_public_keys, | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems unused. Let's remove it. Also, I think it's not a good idea to provide constructors that don't enforce valid object creation. I mean, this method would allow to create a WitnessSet instance with invalid signatures for the given public keys. In case such a method is needed for the rest I'd have it return a Result and return Err in case signatures are not valid for the give public keys, or there's a mismatch in the number of elements (instead of panicking)
There was a problem hiding this comment.
This function is necessary for Keycard. Keycard produces a signature within its logic. Though, it makes sense to add checks for:
- Check signature with corresponding public key.
- Enforce length.
There was a problem hiding this comment.
I added tests for this constructor as well. Let me know what you think.
|
|
||
| // Assumes this now silently skips accounts without signing keys | ||
| let witness_set = WalletCore::sign_public_message(self.0, &message, &sign_ids) | ||
| .expect("`WalletCore::sign_public_message() failed to produce a signature for a NativeTokenTransfer."); |
There was a problem hiding this comment.
| .expect("`WalletCore::sign_public_message() failed to produce a signature for a NativeTokenTransfer."); | |
| .expect("`WalletCore::sign_public_message()` failed to produce a signature for a NativeTokenTransfer."); |
| #[must_use] | ||
| pub fn sign_privacy_message( | ||
| message: &nssa::privacy_preserving_transaction::Message, | ||
| proof: &Proof, |
There was a problem hiding this comment.
let's have proof: Proof as parameter so there's no hidden cloning inside this function
| } | ||
|
|
||
| #[must_use] | ||
| pub fn sign_privacy_message( |
There was a problem hiding this comment.
I don't understand this indireciton here. Why do we need this wrapper of the witnesset constructor?
There was a problem hiding this comment.
I separated the logic in sign_privacy_message to send_privacy_preserving_tx_with_pre_check since Keycard support will introduce more complex logic for signatures.
jonesmarvin8
changed the base branch from
marvin/signature-bip340-fixes
to
main
3 participants
🎯 Purpose
Minor refactoring Wallet's handling of public accounts in preparation of Keycard integration.
⚙️ Approach
KeyTreePublicto optional storageWitnessSetgeneration (specific for native token transactions).🧪 How to Test
All pre-existing tests should be unaffected.
🔗 Dependencies
Uses PR 457 as a base to have a single base for PR 451 to build off of.
🔜 Future Work
N/A
📋 PR Completion Checklist