Allocate error ranges, extend pause coverage, two-step admin transfer - #1229
Merged
yusuftomilola merged 1 commit intoJul 27, 2026
Conversation
…nsfer SC-45: error code allocation - Each contract now owns a numeric range, with shared errors at fixed codes: 1-99 shared, 100-199 assetsup, 200-299 contrib, 300-399 multisig-wallet, 400-499 multisig-transfer, 500-599 asset-maintenance (reserved). - Previously every crate numbered from 1 independently, so code 1 meant AlreadyInitialized in three contracts and NotInitialized in the fourth — the exact opposite condition. Code 5 had four different meanings. A backend could not interpret a bare code without also knowing which contract answered. - Because ranges do not overlap, a code now identifies its origin contract on its own. - Every variant has a doc comment saying when it is returned. - Tests assert the shared codes match across crates and that no contract's variants escape its block. - 48 expected-code assertions in existing tests updated to match. - contrib is left unrenumbered: its error.rs has no mod declaration and is not compiled, so there is nothing to renumber. Its range is reserved. SC-47: pause coverage - The pause covered 4 of ~35 mutating entrypoints. Tokenization, dividends, voting, transfer restrictions, leasing, insurance and detokenization all kept running while the contract reported itself paused, which defeats the control entirely during an incident. - Added the guard to 27 entrypoints via a require_not_paused helper. - Exemptions are deliberate and documented: the pause controls themselves, the admin transfer flow (an incident may be caused by a compromised admin key, so rotating it must not require unpausing first), and claim_dividends (a user exit path; freezing it would trap funds users have already earned). - The key test parses lib.rs and fails if any mutating entrypoint lacks the guard, so a new entrypoint cannot silently escape the pause. Verified it fails when an exemption is removed rather than passing vacuously. - Reads still work while paused; a pause that blocks reads is an outage. SC-48: two-step admin transfer - Replaces update_admin with propose_admin, accept_admin and cancel_admin_proposal, plus a get_pending_admin read. - Acceptance is authorized by the incoming address, so an address that never accepts leaves the original admin in place. Previously one typo permanently bricked administration of a contract governing real asset ownership, with no on-chain undo. - Events on propose, accept and cancel. - 18 tests cover the state machine: proposal does not move the role, a superseded nominee cannot take it, a cancelled proposal cannot be accepted, acceptance is not repeatable, and the old admin loses rights immediately. SC-46: split assessment - contracts/SPLIT-PLAN.md, written rather than implemented, since the issue asks for a plan agreed before any code moves. - Recommends NOT splitting yet. The issue's "8,894 lines across 33 files" counts tests; the contract source is 3,504 lines across 13 files. assetsup builds to 103 KB after the SC-37 profile work, well inside Soroban's limits, so splitting would trade a real cost against a size problem that is not currently binding — and it would add trust boundaries right after an audit found ten entrypoints not checking authorization within a single contract. - The real problem is the assetsup/contrib duplication, and it is smaller than it looks: roughly 1,670 lines of contrib are not compiled at all, so the deployed contract has no escrow, KYC, staking or oracle despite the source files being present. For every concern that does ship in both, assetsup's implementation is the superset. - If a split does go ahead the realistic target is three contracts, not eight: a registry core plus insurance and leasing. The plan records the sequencing, the shared-types crate, and the interface, with open questions for maintainers. Closes DistinctCodes#1209 Closes DistinctCodes#1210 Closes DistinctCodes#1211 Closes DistinctCodes#1212
|
@temiport25 is attempting to deploy a commit to the naijabuz's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@temiport25 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.
Excellent smart contract improvements! Error range allocation improves error handling, pause coverage ensures emergency shutdown capability, and two-step admin transfer enhances security.
9 tasks
portableDD
added a commit
to portableDD/AssetsUp
that referenced
this pull request
Jul 27, 2026
…ents Resolves the conflicts with main. The crate rename made this the largest of them: git detected the directory rename and relocated main's new multisig_transfer files (README.md, auth_tests.rs) into multisig-transfer/, which is the right answer and is accepted as-is. assetsup/src/lib.rs: - initialize gained the typed contract_initialized event here and the TTL extend-on-write from SC-44 on main; kept both. - The two-step admin transfer from SC-48 landed on main emitting three events through the deprecated untyped API this branch removes. SDK 23 deprecates env.events().publish(), and CI runs clippy with -D warnings, so leaving them would fail the build. propose_admin, accept_admin and cancel_admin_proposal now emit typed events, with two new #[contractevent] types added for the steps that had no equivalent: AdminProposed and AdminProposalCancelled. Each step is separately observable, which the old single-step admin_changed could not express. asset-maintenance/src/lib.rs: kept main's crate docs with this branch's import line, which no longer needs symbol_short. add_warranty_information gained the require_admin gate from SC-42 on main and the factored store_warranty helper plus typed event here; kept both. assetsup/src/tests/mod.rs: kept both module lists. Two things the merge surfaced that needed real fixes: main does not currently compile its tests. The auth audit (DistinctCodes#1228) added a test calling update_admin, and the two-step admin transfer (DistinctCodes#1229) removed it. The two merged cleanly line by line and broke each other semantically, which no conflict marker catches. Rewritten against the two-step API and split so both halves are covered. This branch's own event test used the same removed entrypoint; it now asserts a distinct event at each step of the transfer, plus cancellation. Documentation the rename made stale is corrected: the crate and workspace READMEs no longer describe the directory/package mismatch as an open problem, and the SECURITY.md line claiming multisig_transfer has no tests is corrected — it has 10 since DistinctCodes#1228, though the request lifecycle itself is still uncovered. All 393 tests pass, clippy and fmt are clean, all five crates resolve by directory name, and all five build to WASM. The 16 remaining untyped publishes are all in contrib's uncompiled files, which is why the build has no deprecation warnings.
amberly-d
added a commit
to amberly-d/AssetsUp
that referenced
this pull request
Jul 27, 2026
Resolves the conflicts with main. contracts/.gitignore: both sides wanted. Kept the deployments/ ignore from SC-55 and the lcov.info ignore from SC-39. CI: the security and coverage jobs this branch adds were still installing a floating `stable` toolchain. Stable has since moved to 1.97.1, which fails to compile ethnum 1.5.2 in the soroban dependency tree, so both jobs would have failed exactly as every unpinned contracts job on main currently does. They now read the channel from rust-toolchain.toml like the jobs SC-53 already pinned. assetsup/src/tests/auth.rs: main does not currently compile its tests. The auth audit (DistinctCodes#1228) added a test calling update_admin, and the two-step admin transfer (DistinctCodes#1229) replaced update_admin with propose_admin/accept_admin. The two merged cleanly line by line and broke each other semantically, which no conflict marker catches. The test is rewritten against the two-step API and split in two, so both halves of the flow are covered: nominating requires the current admin, and accepting requires the incoming address. It also asserts the rejected proposal is not recorded and the admin is unchanged, which the original did not. All 435 tests pass, clippy is clean, fmt is clean.
TomikeDS
added a commit
to TomikeDS/AssetsUp
that referenced
this pull request
Jul 27, 2026
…test Resolves the conflicts with main. assetsup/src/lib.rs, three hunks, all additive on both sides: - DataKey gained StorageVersion here and PendingAdmin on main; kept both. - initialize gained the version stamp here and the TTL extend-on-write from SC-44 on main; kept both, and added StorageVersion to the extended keys so the new entry gets the same lifetime as the rest of the configuration. - The upgrade/migrate/storage_version entrypoints landed in the same place as main's accept_admin/cancel_admin_proposal/get_pending_admin; kept both, with the admin flow first so it reads continuously from propose_admin. assetsup/src/tests/mod.rs: kept both module lists. multisig_transfer/src/lib.rs: main added caller.require_auth() to create_transfer_request while this branch renamed _admin to admin to use it in the ownership check. Both are needed and both are kept. Two things the merge surfaced that needed real fixes rather than resolution: The pause guard-rail test from SC-47 failed, correctly, flagging upgrade, migrate and storage_version as mutating entrypoints with no pause check. That test is doing exactly its job. upgrade and migrate are deliberately exempt — an upgrade is often how you fix the incident that caused the pause, as UPGRADE.md already documented — so they are recorded in PAUSE_EXEMPT with that reason. storage_version is a read and is classified as one. main does not currently compile its tests: the auth audit (DistinctCodes#1228) added a test calling update_admin, and the two-step admin transfer (DistinctCodes#1229) removed it. The two merged cleanly line by line and broke each other semantically. The test is rewritten against the two-step API and split so both halves are covered. CI: the wasm-size-delta job this branch adds was still installing a floating `stable` toolchain, which is now 1.97.1 and fails to compile ethnum in the soroban tree. It now reads the pinned channel like every other contracts job. All 404 tests pass, clippy and fmt are clean, and every contract is within its WASM budget.
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
2 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 #1209
Closes #1210
Closes #1211
Closes #1212