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

refactor(experimental): add function to check if a transaction is fully signed #1783

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

mcintyre94
Copy link
Collaborator

  • asserts that there is a signature for each expected signer
  • only verifies the presence of signatures, does not verify the signatures are valid

Part of #1773, but won't close yet in case we want to add a separate function to validate signatures too

Copy link
Collaborator Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

Copy link
Collaborator

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Awesome job, just one request: can we get a type test for this? Just something that demonstrates the transaction can now be used as an IFullySignedTransaction.

@mcintyre94
Copy link
Collaborator Author

Yep good shout! Added this:

    // assertTransactionIsFullySigned
    const transaction = {} as Parameters<typeof assertTransactionIsFullySigned>[0];
    // @ts-expect-error Should not be fully signed
    transaction satisfies IFullySignedTransaction;
    assertTransactionIsFullySigned(transaction);
    transaction satisfies IFullySignedTransaction;

Copy link
Collaborator

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

sign it

Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

okey-panter-tost-e.gif

requiredSigners.forEach(address => {
if (!transaction.signatures[address]) {
// TODO coded error
throw new Error(`Transaction is missing signature for address ${address}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I be annoying and ask for this in nice copy-pastable markdown format?

Suggested change
throw new Error(`Transaction is missing signature for address ${address}`);
throw new Error(`Transaction is missing signature for address \`${address}\``);

…ly signed

- asserts that there is a signature for each expected signer
- only verifies the presence of signatures, does not verify the signatures are valid
@mcintyre94 mcintyre94 merged commit 3baf697 into master Oct 26, 2023
6 checks passed
@mcintyre94 mcintyre94 deleted the add-assert-fully-signed-tx branch October 26, 2023 15:10
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.87.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants