Skip to content

Allocate error ranges, extend pause coverage, two-step admin transfer #1229

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.

Sign up for GitHub

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

Merged
yusuftomilola merged 1 commit into from
Jul 27, 2026
Merged

Allocate error ranges, extend pause coverage, two-step admin transfer #1229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions contracts/ERRORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
# Error code allocation

Contract errors cross the network as bare `u32` codes. When two contracts use
the same integer for different meanings, a backend receiving code `3` cannot
say what happened without also knowing which contract produced it — and a
cross-contract call cannot interpret a failure from its callee at all.

This document is the allocation table ([SC-45]).

## The collisions this resolves

Before this change every crate numbered from 1 independently:

| Code | `assetsup` | `contrib` | `multisig-wallet` | `multisig-transfer` |
|---:|---|---|---|---|
| 1 | `AlreadyInitialized` | `AlreadyInitialized` | `AlreadyInitialized` | **`NotInitialized`** |
| 2 | `AdminNotFound` | `AdminNotFound` | **`NotInitialized`** | **`Unauthorized`** |
| 3 | `AssetAlreadyExists` | `AssetAlreadyExists` | **`Unauthorized`** | **`InvalidOwner`** |
| 4 | `AssetNotFound` | `AssetNotFound` | **`InvalidThreshold`** | **`InvalidNewOwner`** |
| 5 | `BranchAlreadyExists` | **`Unauthorized`** | **`InsufficientOwners`** | **`AssetNotFound`** |
| 8 | `Unauthorized` | `InvalidTokenSupply` | `TransactionExpired` | `RuleNotFound` |

Code `1` alone means both "already initialized" and its exact opposite
depending on which contract answered. Code `5` has four different meanings.

## Allocation

Each contract owns a numeric range. Shared errors are defined once, at fixed
codes, and every contract uses those rather than declaring its own.

| Range | Owner |
|---:|---|
| 1–99 | **Shared** — cross-cutting errors every contract may return |
| 100–199 | `assetsup` |
| 200–299 | `contrib` |
| 300–399 | `multisig-wallet` |
| 400–499 | `multisig-transfer` |
| 500–599 | `asset-maintenance` (reserved; see below) |
| 600+ | Unallocated. Claim the next free block here before using it. |

A code, once published, is permanent. Retiring a variant means leaving its
number unused, never reassigning it — a backend built against the old meaning
would silently misinterpret the new one.

## Shared errors (1–99)

Defined once in `assetsup::error::shared`, and mirrored at the same numbers by
every other crate. These are the errors whose meaning is identical everywhere,
so a caller can handle them without knowing which contract replied.

| Code | Variant | Returned when |
|---:|---|---|
| 1 | `AlreadyInitialized` | An initialize entrypoint is called on a contract that already holds state. |
| 2 | `NotInitialized` | Any entrypoint is called before initialization. |
| 3 | `Unauthorized` | The caller authenticated but is not permitted to perform this action. |
| 4 | `InvalidInput` | An argument failed validation and no more specific code applies. |
| 5 | `NotFound` | A referenced entity does not exist and no more specific code applies. |
| 6 | `ContractPaused` | A mutating entrypoint was called while the emergency pause is active. |
| 7 | `MathOverflow` | An arithmetic operation would exceed the type's range. |
| 8 | `MathUnderflow` | A subtraction would go below the type's minimum. |

Note the distinction between codes 1 and 2, which is where the old numbering
was at its worst: `AlreadyInitialized` and `NotInitialized` previously shared
code 1 across different contracts.

## `assetsup` (100–199)

| Block | Concern |
|---:|---|
| 100–119 | Registry: assets, branches, registrars |
| 120–139 | Tokenization and balances |
| 140–149 | Voting |
| 150–159 | Dividends |
| 160–169 | Detokenization and valuation |
| 170–179 | Validation |
| 180–199 | Leasing and insurance |

## `contrib` (200–299)

`contrib` has **no typed errors in compiled code**. Its `src/error.rs` defines
an enum, but the file has no `mod` declaration, so nothing references it and
every failure surfaces as a `panic!` on a string. The range is reserved for
when that module is wired in or removed as part of [SC-46].

## `multisig-wallet` (300–399)

| Block | Concern |
|---:|---|
| 300–319 | Transaction lifecycle |
| 320–339 | Owner and threshold governance |
| 340–349 | Emergency controls and limits |

## `multisig-transfer` (400–499)

| Block | Concern |
|---:|---|
| 400–419 | Request lifecycle |
| 420–439 | Approval rules and approvers |
| 440–449 | Registry interaction |

## `asset-maintenance` (500–599)

Reserved but unused. This crate has **no error enum at all** — it raises
failures with `panic!` on a `&str`, so callers cannot distinguish a missing
warranty from an inactive provider by code. Converting it to a `contracterror`
in the 500 range is follow-up work; the range is claimed here so it does not
get taken in the meantime.

## For backend implementers

```
code < 100 → shared meaning, safe to handle generically
100 <= code < 200 → assetsup-specific
200 <= code < 300 → contrib-specific
300 <= code < 400 → multisig-wallet-specific
400 <= code < 500 → multisig-transfer-specific
500 <= code < 600 → asset-maintenance-specific
```

Because the ranges do not overlap, a code identifies its origin contract on its
own. That is the property the old numbering lacked and the reason for the
renumbering.

## Adding an error

1. Decide whether it is genuinely shared. If two contracts would return it with
the same meaning, it belongs in 1–99 and must be added to every crate's enum
at the same number.
2. Otherwise take the next free code **within your contract's block**, not the
next free code overall.
3. Give it a doc comment saying when it is returned. Every variant has one; a
code with no stated meaning is not usable by a caller.
4. Never reuse a retired code.
198 changes: 198 additions & 0 deletions contracts/SPLIT-PLAN.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
# Splitting `assetsup`: assessment and migration plan

Assessment for [SC-46]. **No code is moved by this change.** The issue asks for
a plan agreed before implementation, and that is what this is.

## The numbers, corrected

The issue describes `assetsup` as "8,894 lines across 33 files". That figure
counts tests. The split-relevant number is smaller:

| | Files | Lines |
|---|---:|---:|
| `assetsup` contract source | 13 | 3,504 |
| `assetsup` tests | 22 | 6,238 |
| **Total** | **35** | **9,742** |

So the contract itself is ~3,500 lines, not ~8,900. Two thirds of the crate is
test code, which is a good sign rather than a problem, and it materially
changes the case for splitting: this is a large module, not an unmanageable
one.

Contract source by module:

| Module | Lines | Concern |
|---|---:|---|
| `lib.rs` | 1,063 | Entrypoint facade for everything below |
| `insurance.rs` | 518 | Policies and the claim state machine |
| `tokenization.rs` | 475 | Fractional shares, balances, locks |
| `error.rs` | 256 | Error enum |
| `types.rs` | 227 | Shared types |
| `lease.rs` | 226 | Lease lifecycle |
| `detokenization.rs` | 198 | Detokenization proposals |
| `transfer_restrictions.rs` | 159 | Whitelists |
| `voting.rs` | 153 | Weighted voting |
| `dividends.rs` | 118 | Distribution and claims |
| `audit.rs` | 53 | Audit log |
| `asset.rs` | 39 | Asset struct and registry keys |
| `branch.rs` | 19 | Branch records |

## Recommendation

**Do not split `assetsup` into multiple contracts yet.** Resolve the
`assetsup`/`contrib` duplication first, then reassess against a measured WASM
size problem rather than an assumed one.

The reasoning:

1. **There is no size emergency.** `assetsup` builds to 103 KB with the
hardened release profile from [SC-37], down from 163 KB. That is
comfortably inside Soroban's limits. Splitting to solve a size problem that
is not currently binding trades a real cost for a speculative benefit.

2. **Splitting multiplies the attack surface it is meant to reduce.** Today the
asset registry and the modules acting on it share one storage space and one
atomic transaction boundary. Separate contracts mean cross-contract calls,
which means each callee must independently verify the caller — and the
authorization audit in [SC-42] found ten entrypoints that were not checking
authorization *within a single contract*. Adding trust boundaries before
that class of bug is under control makes things worse, not better.

3. **The real coupling problem is `assetsup` versus `contrib`, not
`assetsup`'s internals.** Two contracts implement `audit`, `insurance` and
`lease` differently, and nobody can say which is authoritative. That is a
correctness and ownership problem today. Module boundaries inside
`assetsup` are a code-organization problem, which is much less urgent.

4. **Nothing can be upgraded piecemeal regardless.** The issue cites piecemeal
upgrade as a motivation, but no contract in the workspace exposes an upgrade
entrypoint at all ([SC-49]). Splitting does not deliver that benefit until
upgradeability exists.

## What to do first: resolve the duplication

`assetsup` and `contrib` are independent contracts with separate storage that
share several module names. The overlap is duplicated code, not a shared
library.

An important finding while mapping the boundaries: **most of `contrib` is not
compiled.** `contrib/src/lib.rs` declares only `audit`, `pause`, `types`,
`insurance` and `lease`. The escrow, KYC, staking, oracle, tokenization,
detokenization, transfer-restriction and `error.rs` files have no `mod`
declaration and are not part of the crate — about 1,670 lines that never ship.
The deployed `ContribContract` therefore has no escrow, no KYC, no staking, no
oracle, and no typed errors, despite the source files sitting right there.

That reframes the whole question. The overlap is much smaller than it appears:

| Concern | `assetsup` | `contrib` (compiled) | Proposed owner |
|---|---|---|---|
| Asset registry | ✅ richer | ✅ separate copy | **`assetsup`** — it is the authoritative registry |
| Audit log | ✅ | ✅ | **`assetsup`** |
| Insurance | ✅ full claim state machine | smaller policy/claim store | **`assetsup`** — `contrib`'s is a strict subset |
| Leasing | ✅ richer lifecycle | check-in/cancel only | **`assetsup`** — same |
| Emergency pause | ✅ (now full coverage, [SC-47]) | ✅ dedicated module | **`assetsup`** |
| Tokenization, dividends, voting, detokenization | ✅ | ❌ dead files | **`assetsup`** |
| Escrow, KYC, staking, oracle | — | ❌ dead files | **Undecided** — see below |

For every concern that actually ships in both, `assetsup`'s implementation is
the superset. There is no case where `contrib`'s compiled version is the better
one to keep.

### Proposed resolution

1. **Decide what `contrib` is for.** Two honest options:
- *Delete it.* Everything it compiles today, `assetsup` already does better.
- *Make it the second contract it was evidently meant to be* by wiring in
escrow, KYC, staking and the oracle — the capabilities `assetsup` genuinely
lacks. This needs a security review of code that has never compiled;
`tokenization.rs` does not even parse as valid contract code.

This is a maintainer decision, not one to make inside a refactor.

2. **Whichever is chosen, delete the dead files.** Leaving 1,670 lines of
uncompiled source that looks like working code is actively misleading —
every reader so far, including the issue that prompted this assessment, has
assumed `contrib` owns escrow and KYC.

3. **Only then** reassess splitting `assetsup`.

## If the split does go ahead

Recorded so the decision does not have to be reconstructed later.

### What is a separate contract versus a shared internal

Genuinely separate — distinct lifecycles, distinct principals, meaningful
alone:

| Candidate | Lines | Why it stands alone |
|---|---:|---|
| **Insurance** | 518 | Insurers and claimants are not asset owners. The claim state machine is self-contained. The largest single module and the weakest coupling to the registry. |
| **Leasing** | 226 | Lessor/lessee lifecycle is independent of ownership; a lease does not change the owner. |

Not separable — they are the registry, or meaningless without its state:

| Module | Why it stays |
|---|---|
| `asset`, `branch`, `audit` | These *are* the registry. |
| `tokenization` | Share balances are the registry's ownership model in fractional form. |
| `dividends`, `voting`, `detokenization` | All read token balances on every call. Splitting turns each into a cross-contract read. |
| `transfer_restrictions` | Consulted inside every transfer; a hot path. |

So the realistic target is **three contracts, not eight**: a registry core,
insurance, and leasing.

### Shared types

Extract into a `assetsup-types` library crate (`crate-type = ["lib"]`, never
deployed) rather than duplicating: `AssetStatus`, `AssetType`, `ActionType`,
`CustomAttribute`, and the shared error range 1–99 from
[`ERRORS.md`](ERRORS.md). The error allocation already anticipates this — each
contract owns a numeric block, so a split does not create code collisions.

### Interfaces

Insurance and leasing both need "does this asset exist and who owns it". That
is one read entrypoint on the registry:

```rust
fn get_asset_info(env: Env, asset_id: BytesN<32>) -> Result<AssetInfo, Error>;
```

Neither needs to *write* to the registry, which keeps the trust relationship
one-directional and avoids the confused-deputy risk that a write interface
would introduce.

### Migration sequence

Each step keeps `cargo test --all` green:

1. Extract `assetsup-types`; both crates depend on it. No behaviour change.
2. Create `assetsup-insurance` with the registry address stored at init. Move
`insurance.rs` **and its tests** together.
3. Add the cross-contract read and integration tests covering the seam
(registry → insurance), building on [SC-51].
4. Remove insurance entrypoints from `assetsup`. **This is the breaking step** —
it changes the deployed ABI, so it needs the upgrade story from [SC-49] to
exist first, or a coordinated redeploy.
5. Repeat for leasing.
6. Record WASM size before and after each step. If the registry core does not
shrink meaningfully, stop — the split is not paying for itself.

### Expected size effect

Insurance and leasing are 744 lines of 3,504, roughly 21% of the source. A
proportional saving would take the registry core from 103 KB to about 81 KB,
before accounting for the cross-contract call machinery each new contract adds
back. The saving is real but modest, which is the main argument for waiting
until there is a size problem worth solving.

## Open questions for maintainers

1. Delete `contrib`, or wire in its dead modules? Everything else waits on this.
2. Is 103 KB actually near a limit you are worried about, or is the size
concern precautionary?
3. Does the backend bridge need to treat insurance and leasing as separate
deployments, or is one contract address simpler for it?
4. Is there an upgrade story planned ([SC-49])? Step 4 above is blocked on it.
Loading
Loading