fix: add storage non-mutation coverage to multisig_approval_init guar… - #425
Open
Yerimahjr wants to merge 1 commit into
Open
fix: add storage non-mutation coverage to multisig_approval_init guar…#425Yerimahjr wants to merge 1 commit into
Yerimahjr wants to merge 1 commit into
Conversation
…d tests (Goldii-locks#353) Also fixes an unrelated pre-existing compile break in admin_override_cancel_tests.rs (missing #![cfg(test)] gate and inaccessible setup_funded_escrow helper) so the crate's test suite can build and run at all.
|
@Yerimahjr Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Author
|
Please review and merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #353
Summary
multisig_approval_init's authorization (require_admin) and precondition(
AlreadyInitialized) guards were already correctly implemented and orderedbefore any storage read/write — verified by comparing against this repo's
own established pattern for this exact class of issue (prior closed
harden caller authorization and precondition guards in <function>issuesfor
admin_override_cancel_releaseandmultisig_split_refund).What was missing, per the issue's own acceptance criteria — "that no storage
entry is mutated in either case" — was test coverage proving it.
Changes
contracts/milestone-escrow/src/test.rsStrengthened the two existing guard tests to additionally assert no storage
mutation on rejection:
test_multisig_approval_init_unauthorized_failstest_multisig_approval_init_duplicate_fails(the "illegal source state"case — a second call to this one-time-init function)
Each now also confirms, after the expected typed error:
is_multisig_approved(0).thresholdis still the original value, not theattacker's attempted one.
confirmed by calling
multisig_approveas them and gettingUnauthorized.contracts/milestone-escrow/src/admin_override_cancel_tests.rsUnrelated pre-existing bug, fixed so the crate's test suite can compile and
run at all: this file (added by a separately-merged PR closing #383/#386)
was missing
#![cfg(test)]— every other test module in this crate has it —so it compiled unconditionally instead of only under
cfg(test), makingsoroban_sdk::testutils::Address::generateunavailable and the siblingtestmodule'ssetup_funded_escrowhelper inaccessible. Added the missinggate, the missing
testutils::Address as _import, and madesetup_funded_escrowpub(crate).Test results
running 463 tests
test result: FAILED. 454 passed; 9 failed
The 454 passed include both target tests for this issue, and every other
test in the crate — this is effectively the crate's first-ever successful
compile+run, since it could not build before this branch. The 9 failures
are all pre-existing, in
admin_override_cancel_tests::test_cancel_refund_*(testing
admin_override_cancel_refund, issue #386 — a different functionentirely), never having run before now. Root cause:
admin_override_cancel_refundcalls
admin.require_auth()redundantly beforerequire_admin(whichalready does so internally) — the same bug already fixed in the sibling
admin_override_cancel_release, just missed here. Flagging as a separateissue rather than bundling an unrelated fix into this PR.
cargo fmt --check: not re-verified after this session's edits — pleaseconfirm in CI.
Note on
contracts/reportsThis repo also contains a duplicate
multisig_approval_initimplementationin
contracts/reports/src/lib.rs, which has the same guards already correct.However,
contracts/reportsis not a member of the workspace (Cargo.tomlworkspace.membersonly listscontracts/milestone-escrow, and always has,since the very first commit), and its package is literally named
milestone-escrowinternally — a name collision that blocks simply addingit as a workspace member. It cannot currently be built or tested at all, so
no changes were made there. Flagging for maintainers to decide whether it
should be wired into the workspace (under a different name) or removed.