-
Notifications
You must be signed in to change notification settings - Fork 6
feat!: introduces multisignature abstractions in utils #218
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR introduces multisignature abstractions in the AlgoKit utilities by adding a new multisig module and renaming existing signer classes to be more specific about their purpose. The changes support both key pair and multisig account signing patterns.
Key changes:
- Adds new
multisig.rs
module withMultisigAccount
and related types - Renames
EmptySigner
toEmptyKeyPairSigner
andTestAccount
toTestKeyPairAccount
for clarity - Introduces
EmptyMultisigSigner
for multisig transaction signing
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
crates/algokit_utils/src/multisig.rs |
New module defining multisig account abstractions and conversions |
crates/algokit_utils/src/transactions/common.rs |
Adds multisig signer implementation and renames existing signer |
crates/algokit_utils/src/transactions/mod.rs |
Updates exports to reflect renamed signer |
crates/algokit_utils/src/transactions/composer.rs |
Updates usage of renamed signer |
crates/algokit_utils/src/testing/account_helpers.rs |
Renames TestAccount to TestKeyPairAccount throughout |
crates/algokit_utils/src/testing/fixture.rs |
Updates to use renamed account type |
crates/algokit_utils/src/testing/mod.rs |
Updates exports to reflect renamed account type |
crates/algokit_utils/src/lib.rs |
Adds multisig module and updates public exports |
Comments suppressed due to low confidence (1)
crates/algokit_utils/src/multisig.rs:45
- The struct
MultisigSignature
has the same name as the existingalgokit_transact::MultisigSignature
type used elsewhere in the codebase. This creates naming ambiguity and could lead to confusion. Consider renaming it to something more specific likeMultisigSignatureBuilder
orPartialMultisigSignature
.
pub struct MultisigSignature {
Ok(msig_signature.into()) | ||
} | ||
} | ||
|
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.
The MultisigSignature
struct lacks documentation explaining its purpose and how it differs from algokit_transact::MultisigSignature
. Add a docstring explaining when and how this struct should be used.
/// Represents a multisignature with optional subsignatures for each participant. | |
/// | |
/// This struct is distinct from `algokit_transact::MultisigSignature` in that it | |
/// focuses on the subsignature details, allowing for partial or incomplete | |
/// multisignatures. It is used when working with multisignature accounts where | |
/// some participants may not have signed yet. | |
/// | |
/// Use this struct when you need to manage or inspect the state of individual | |
/// subsignatures in a multisignature account. |
Copilot uses AI. Check for mistakes.
@@ -52,7 +53,47 @@ impl TransactionSigner for EmptySigner { | |||
} | |||
} | |||
|
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.
The EmptyMultisigSigner
struct lacks documentation explaining its purpose and intended use case. Add a docstring describing when this signer should be used and how it behaves.
/// A signer implementation for multisignature accounts that produces placeholder signatures. | |
/// | |
/// # Purpose | |
/// This struct is intended for use in testing or scenarios where real signatures are not required. | |
/// It creates a multisignature with empty subsignatures (`[0; ALGORAND_SIGNATURE_BYTE_LENGTH]`). | |
/// | |
/// # Behavior | |
/// When signing transactions, it populates the multisignature with placeholder subsignatures | |
/// and does not produce a valid cryptographic signature. This makes it unsuitable for | |
/// production use but useful for testing or as a placeholder. |
Copilot uses AI. Check for mistakes.
multisig | ||
.subsignatures | ||
.iter_mut() | ||
.for_each(|subsig| subsig.signature = Some([0; ALGORAND_SIGNATURE_BYTE_LENGTH])); |
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.
Setting all subsignatures to zero bytes creates an invalid multisig signature. This could be misleading for testing purposes. Consider adding a comment explaining that this creates dummy signatures for testing, or provide a way to create valid empty signatures.
multisig | |
.subsignatures | |
.iter_mut() | |
.for_each(|subsig| subsig.signature = Some([0; ALGORAND_SIGNATURE_BYTE_LENGTH])); | |
// Set dummy signatures for testing purposes. These are not valid signatures | |
// and should only be used in non-production scenarios. | |
multisig | |
.subsignatures | |
.iter_mut() | |
.for_each(|subsig| subsig.signature = Some(Self::generate_dummy_signature())); |
Copilot uses AI. Check for mistakes.
No description provided.