Add upgrade path, property tests, cross-contract tests, WASM budgets - #1230
Open
TomikeDS wants to merge 3 commits into
Open
Add upgrade path, property tests, cross-contract tests, WASM budgets#1230TomikeDS wants to merge 3 commits into
TomikeDS wants to merge 3 commits into
Conversation
|
@TomikeDS is attempting to deploy a commit to the naijabuz's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@TomikeDS 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! 🚀 |
yusuftomilola
approved these changes
Jul 27, 2026
yusuftomilola
left a comment
yusuftomilola
left a comment
Collaborator
There was a problem hiding this comment.
Comprehensive quality improvements! Upgrade path planning, property-based tests, cross-contract integration tests, and WASM budget tracking all demonstrate strong engineering practices.
yusuftomilola
requested changes
Jul 27, 2026
yusuftomilola
left a comment
yusuftomilola
left a comment
Collaborator
There was a problem hiding this comment.
Hi @TomikeDS, 👋
This PR has merge conflicts that need to be resolved before it can be merged. The following files have conflicts:
contracts/assetsup/src/lib.rs
contracts/assetsup/src/tests/mod.rs
contracts/multisig_transfer/src/lib.rs
Please resolve these conflicts in your branch and push the updates. Once resolved, the PR can be merged successfully.
Thank you!
yusuftomilola
approved these changes
Jul 27, 2026
yusuftomilola
left a comment
yusuftomilola
left a comment
Collaborator
There was a problem hiding this comment.
Comprehensive quality improvements! Upgrade path planning, property-based tests, cross-contract integration tests, and WASM budget tracking all demonstrate strong engineering practices.
yusuftomilola
requested changes
Jul 27, 2026
yusuftomilola
left a comment
yusuftomilola
left a comment
Collaborator
There was a problem hiding this comment.
Hi @TomikeDS, 👋
This PR has merge conflicts that need to be resolved before it can be merged. The following files have conflicts:
.github/workflows/CI.yaml
contracts/Cargo.lock
contracts/assetsup/src/lib.rs
contracts/multisig-transfer/src/integration_tests.rs
contracts/multisig-wallet/src/lib.rs
Please resolve these conflicts in your branch and push the updates. Once resolved, the PR can be merged successfully.
Thank you!
SC-51 turned up the most serious finding, so it comes first. SC-51: cross-contract integration tests - multisig_transfer/src/registry.rs was entirely stubbed. asset_exists returned true unconditionally, asset_is_retired returned false, get_owner always errored, and transfer_owner did nothing and returned Ok. So execute_transfer reported success while never moving ownership — the entire purpose of the contract was a no-op, and per-crate tests could not see it because the seam was never exercised. - Implemented the calls for real via Env::invoke_contract, keeping the crates decoupled so either can be redeployed independently. - Wired up the ownership check in create_transfer_request that was left as a TODO "omitted here due to stub", which meant anyone could raise a transfer request against any asset. - Added 8 integration tests registering the real assetsup registry alongside this contract in one Env: a fully approved transfer moves ownership in the other contract's storage; a transfer below the threshold leaves ownership untouched with no partial state and can still complete later; an unauthorized approver is rejected and does not count toward the threshold; unregistered and retired assets are rejected; a non-owner cannot raise a request; and a completed transfer is visible in both contracts. - Documented the deployment consequence: governed assets must be owned by the multisig contract's address, since it passes its own address as caller. SC-49: upgrade path - assetsup is upgradeable via upgrade(new_wasm_hash), admin-gated and emitting an event. It is the asset registry, and redeploying would mean a new contract id and abandoning every ownership record. - The other four are documented as immutable-and-redeploy, deliberately rather than by omission, with the reasoning per contract. - Added a storage version key and an idempotent migrate(), so a retried or duplicated migration cannot corrupt state. Migrating from a version newer than the running build is refused. - upgrade and migrate both work while paused: an upgrade is often how you fix the incident that caused the pause. - contracts/UPGRADE.md carries the runbook, the ordering constraint (upgrade then migrate, since the migration code ships in the new WASM), and the blast-radius trade — a compromised admin key can replace the contract wholesale, which is the argument for holding admin in the multisig wallet. - 7 tests. The WASM swap itself is not unit-tested: the test env registers contracts natively, so update_current_contract_wasm cannot run there. That is stated rather than papered over, and the runbook covers it on testnet. SC-50: property-based tests - 5 multisig properties: confirmation count always equals the number of distinct current owners who confirmed, a repeated confirmation never raises it, a proposal never executes below its threshold, the threshold stays within 1..=owner_count after any accepted governance change, and removals never strand the threshold. - 5 share-accounting properties: holder balances always sum to total supply after any sequence of mint, burn and transfer; minting and burning move supply by exactly the amount; an overdrawn transfer changes nothing; and no balance ever goes negative. - Case counts kept low since each case builds a fresh Soroban Env. SC-52: WASM size budgets - scripts/check-wasm-size.sh reports per-contract size against a recorded baseline and a budget, and fails the build when one is exceeded. A new contract with no budget fails rather than passing silently. - CI builds all five contracts for wasm32, enforces the budgets, and uploads the artifacts. - A separate job comments the per-contract size delta against the base branch on each PR, updating its own comment rather than adding new ones. - Baselines recorded in scripts/wasm-size-baseline.txt; budgets sit ~25% above measured so ordinary work does not trip them but a jump does. assetsup's `asset` module is now public: `Asset` is already part of the public ABI as a register_asset argument, and the integration tests need to construct one. Closes DistinctCodes#1213 Closes DistinctCodes#1214 Closes DistinctCodes#1215 Closes DistinctCodes#1216
The job failed with "Resource not accessible by integration". A pull_request event from a fork gets a read-only GITHUB_TOKEN whatever the permissions block declares — that is a GitHub security boundary, not a misconfiguration — so the comment API call could never succeed for exactly the PRs this repository receives. Reworked so the delta is always produced and always visible: - The table is written to the job summary, which needs no token and shows up in the checks UI for every PR including forks. That is now the primary output. - The PR comment is best-effort: skipped entirely for fork PRs, and marked continue-on-error so a token or API problem cannot fail the build over a convenience feature. Switching to pull_request_target would restore write access, but it runs with a privileged token against the PR's own code, which is not a trade worth making for a size report. The table is built in Python rather than inline JS so it renders identically in the summary and the comment. Verified against grown, shrunk and newly added contracts.
…sting-library/react
TomikeDS
force-pushed
the
feat/sc-49-52-upgrade-proptest-integration-wasm
branch
from
July 27, 2026 17:27
b9b73f6 to
be00944
Compare
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
3 participants
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 #1213
Closes #1214
Closes #1215
Closes #1216