feat(standards): add foundation for Multisig Smart Authentication Component#2806
feat(standards): add foundation for Multisig Smart Authentication Component#2806onurinanc wants to merge 6 commits into0xMiden:nextfrom
Conversation
mmagician
added
standards
partylikeits1983
left a comment
partylikeits1983
left a comment
There was a problem hiding this comment.
Looks good! Just a few comments during my first review pass.
I saw some MASM that I saw could be cycle count optimized so I went to check how many cycles could be shaved off in the set_procedure_policy procedure, but then realized that the tests do not call it.
I'd push for adding tests which execute all MASM which is added. I understand this is a foundational PR for follow up PRs, but even minimal testing would be better than no tests. I think once this has a bit more test coverage it will be ready to merge, because everything else looks good!
|
|
||
|
|
There was a problem hiding this comment.
Nit: double empty line
| #! | ||
| #! Invocation: exec | ||
| @locals(3) | ||
| pub proc set_procedure_policy |
There was a problem hiding this comment.
I could not find a test which calls set_procedure_policy. Could we add a test which calls it just so we don't add code which is never tested.
| #! | ||
| #! Invocation: call | ||
| @locals(2) | ||
| pub proc update_signers_and_threshold(multisig_config_hash: word) |
There was a problem hiding this comment.
Same here, can we add a test which calls this?
| (Vec<AuthSecretKey>, Vec<AuthScheme>, Vec<PublicKey>, Vec<BasicAuthenticator>); | ||
|
|
||
| /// Sets up secret keys, auth schemes, public keys, and authenticators for a specific scheme. | ||
| fn setup_keys_and_authenticators_with_scheme( |
There was a problem hiding this comment.
Is it not possible to reuse the existing setup_keys_and_authenticators_with_scheme function which we use in the existing mutlisig?
3 participants
This is a follow-up PR for: #2670
It's a foundation for MultisigSmart Authentication Component will have the following properties:
The completed version of the PR having the above features is a big PR, so, decided to separate these PRs into 3:
I believe this PR is self-explanatory since it doesn't include any features but prepares a core component for MultisigSmart.