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

[BCI 2027]: Adds multisig cairo tests #359

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

chris-de-leon-cll
Copy link
Contributor

@chris-de-leon-cll chris-de-leon-cll commented Feb 14, 2024

This PR adds some new test cases for the following functions:

  1. set_threshold
  2. set_signers
  3. set_signers_and_threshold

For each function above, there are 2 new corresponding test cases: a recursive version and a non-recursive version.

In the recursive version, a transaction is created which calls one of the Multisig functions above. This transaction is submitted to the Multisig contract, which is then confirmed by the correct number of signers and executed. The test case verifies the state before and after the transaction execution.

In the non-recursive version, the Multisig contract is deployed, and the methods on it are directly called using a dispatcher. There is no transaction submission/confirmation. The test simply calls a function and verifies that the result is correct.
I believe the recursive version is what the ticket here is referring to, but I included both versions for completeness.

@archseer
Copy link
Collaborator

Can you also drop packages-ts/integration-multisig? These changes should now obsolete them :)

Also delete the mention in the makefile:

chainlink-starknet/Makefile

Lines 224 to 225 in 1272736

cd packages-ts/integration-multisig/ && \
yarn test

@chris-de-leon-cll
Copy link
Contributor Author

@archseer I've deleted the packages-ts/integration-multisig folder and updated the Makefile accordingly - feel free to approve and merge if everything else looks good!

Copy link
Collaborator

@archseer archseer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working on this!

@archseer archseer merged commit 00cac1d into update-dependencies Feb 19, 2024
11 of 18 checks passed
@archseer archseer deleted the feat/BCI-2027-multisig-cairo-tests branch February 19, 2024 07:57
@chris-de-leon-cll chris-de-leon-cll changed the title [BCI 2027: Adds multisig cairo tests [BCI 2027]: Adds multisig cairo tests Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants