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

feat/modify rotate key to ask for all signatures #1315

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Jiloc
Copy link
Collaborator

@Jiloc Jiloc commented Feb 6, 2025

Description

Make sure that rotate keys is signed by all signers.

Changes

  • modify estimate_fees to accept a new parameter require_all_signatures that forces the estimate to expect all signers signatures.
  • modify coordinator sign_stacks_transaction to wait for all signatures in case of a RotateKeyTx.

Testing Information

  • extended unit tests for estimate_fees

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Jiloc Jiloc requested review from djordon, matteojug and cylewitruk and removed request for matteojug February 6, 2025 16:28
signer/src/stacks/wallet.rs Outdated Show resolved Hide resolved
signer/src/transaction_coordinator.rs Outdated Show resolved Hide resolved
signer/src/transaction_coordinator.rs Show resolved Hide resolved
signer/src/transaction_coordinator.rs Show resolved Hide resolved
Copy link
Collaborator

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Looks good. If you have the time, I would add a test that an honest coordinator would not submit a rotate-keys contract call transaction if one of the signers does not send a signature.

@Jiloc
Copy link
Collaborator Author

Jiloc commented Feb 7, 2025

Looks good. If you have the time, I would add a test that an honest coordinator would not submit a rotate-keys contract call transaction if one of the signers does not send a signature.

I added a test, but I don't like how I did it :( Open to suggestions if you have a better way in mind. Expecially to avoid exposing the config_mut() to force a validation to fail on one of the signers

@Jiloc Jiloc self-assigned this Feb 7, 2025
@Jiloc Jiloc added this to the sBTC: Deposits milestone Feb 7, 2025
Copy link
Collaborator

@matteojug matteojug left a comment

Choose a reason for hiding this comment

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

There may be an issue with this feature (blocking merge pending confirmation)

Comment on lines +207 to +212
/// Returns true if the transaction is a rotate keys transaction, regardless
/// of the version.
pub fn is_rotate_keys(&self) -> bool {
matches!(self, StacksTx::ContractCall(ContractCall::RotateKeysV1(_)))
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Enum variant name RotateKeysV1 and "regardless of the version" comment looks a bit confusing. Maybe something like

/// Returns true if the transaction is a rotate keys transaction, regardless
/// of it's contens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants