fix(station): make asset ledger canister id immutable after creation - #648
MRmarioruci wants to merge 9 commits into
Conversation
The ledger canister id identifies the token an asset represents. It is resolved from mutable asset metadata at call time by both transfer paths as well as by balance and fee lookups, so changing it on an existing asset silently changes which token those operations act on. Reject edits that repoint or remove the ledger canister id once it is set. Pointing an asset at a different ledger now requires detaching it from its accounts, removing it, and creating a new one, which is the correct semantics anyway since balances do not follow the pointer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to 8833539. Security OverviewDetected Code Changes
|
There was a problem hiding this comment.
Pull request overview
This PR makes an asset’s ledger_canister_id effectively immutable after the asset has been created, preventing previously-approved transfers (and balance/fee lookups) from being redirected to a different ledger by later metadata edits.
Changes:
- In
AssetService::edit(), capture the pre-editledger_canister_idand reject edits that remove or change it once it has been set. - Introduce
AssetError::ImmutableLedgerCanisterIdfor the new rejection case. - Add unit tests covering repointing/removal rejection and allowing unrelated metadata edits.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| core/station/impl/src/services/asset.rs | Enforces immutability of ledger_canister_id during asset edits and adds regression tests. |
| core/station/impl/src/errors/asset.rs | Adds a dedicated error variant for attempted ledger canister id changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🔵 Needs a closer look
Add regression coverage for assigning a ledger ID to an asset that previously lacked one.
Review details
Suppressed comments (1)
core/station/impl/src/services/asset.rs:116
- The
before.is_none()exception is part of the contract, but the added tests only exercise assets that already have a ledger. Please add a regression test starting frommock_asset_b()that setsledger_canister_idand verifies the edit succeeds and persists; otherwise a future refactor could make the first assignment fail without detection.
if ledger_canister_id_before.is_some()
&& ledger_canister_id_before != ledger_canister_id_after
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Introduces AssetValidationError with an ImmutableField variant and wires it through ValidationError, RequestError and RequestPolicyError. Adds Asset::changes_ledger_canister_id so the request validator and the asset service share one definition of what counts as a ledger repoint.
EditAsset only checked that the asset existed, so an edit repointing the ledger canister id was accepted, approved, and only refused by the service at execution. The check now runs in RequestOperation::validate as well, and the service reuses the shared helper instead of comparing metadata before and after applying the edit.
is_external_canister derives the ledger set from current asset metadata, so a ledger stops being recognised once the last asset naming it is edited or removed. Pin the ICP and cycles ledgers alongside the management canister, the station and the upgrader so they no longer depend on registry contents.
The immutability guard deliberately allows the first assignment, since an asset without a ledger cannot transact, but only assets that already had one were exercised. Adds coverage on both the service and the request validator starting from an asset with empty metadata, so a refactor cannot make the first assignment fail unnoticed.
|
Added in 36d2f4a. Good catch: the first-assignment path was contract but untested. Covered on both paths, each starting from
Confirmed the coverage is real by removing the |
There was a problem hiding this comment.
🔵 Needs a closer look
Initial ledger canister IDs are not validated as Principals before becoming immutable.
Review details
Suppressed comments (1)
core/station/impl/src/models/asset.rs:55
- This allows the first write to
ledger_canister_idwithout validating that the value is a Principal. A typo such asnot-a-principalis accepted by the metadata length checks, after which transfers/balance calls fail withInvalidMetadataand this new immutability rule prevents correcting or removing the value. Validate the initial ledger canister id before making it immutable (and return a validation error to the caller).
if before.is_none() {
return false;
}
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
The value is only parsed when a transfer or balance read resolves it, so an unparseable one was accepted at write time and surfaced much later as InvalidMetadata. Since it also cannot be corrected in place once set, a typo had to be undone by detaching the asset from its accounts and recreating it. Asset validation now parses it, which covers both creation and the first assignment on an existing asset.
|
Added in 847a975. Confirmed the gap:
Tests: a principal is accepted, a non-principal is rejected, an asset without one still validates, and at the service level an unparseable first assignment is refused and nothing is persisted. 407 lib tests pass. One scoping note: this validates the ledger canister id specifically, not metadata values generally. Typed metadata validation by key is a broader change and belongs on its own. |
There was a problem hiding this comment.
🟡 Changes recommended
Raw textual comparison can still misclassify valid non-canonical custom ledger principals as external canisters.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
core/station/impl/src/models/request_operation.rs:1255
- The helper tests cover the comparison logic, but no test exercises this
RequestOperation::validatebranch for anEditAssetrepoint or removal. This dispatch could therefore be dropped while the current tests remain green, allowing requests that can only fail after approval. Add a request-operation or request-creation test that inserts an asset, attempts the edit, and asserts the immutable-field validation error.
EnsureAsset::ledger_canister_id_preserved(
&op.input.asset_id,
&op.input.change_metadata,
)?;
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
from_text accepts spellings that to_text does not emit, so an asset holding a ledger canister id in one of those forms compared unequal and left that ledger treated as an external canister. Parse the stored value and compare principals, which also covers records already written that way. Adds the missing coverage for the EditAsset branch of RequestOperation::validate.
|
Both addressed in 8833539. Principal vs text comparison. Confirmed and fixed. Worth noting this only affected custom ledgers. The ICP and cycles ledgers were already compared as principals. Missing coverage on the validate branch. Fair, the dispatch could have been deleted with everything still green. Added 409 lib tests pass. Clippy and fmt clean. |
Change
An asset's
ledger_canister_ididentifies the token that asset represents. It is resolved from asset metadata at call time by both transfer paths and byaccount_balance,icrc1_balance_of,transfer_feeandicrc1_fee, so changing it on an existing asset changes which token all of those operations act on.EditAssetnow rejects an edit that repoints or removes it once set, at request creation as well as in the service. Setting one that was previously absent is still allowed, since an asset without a ledger cannot transact.The same metadata also feeds
EnsureExternalCanister::is_external_canister, which decides whether a canister may be registered and called as an external canister. Because that set is derived from current asset metadata, a ledger stopped being recognised as one once the last asset naming it went away. The ICP and cycles ledgers are now pinned alongside the management canister, the station and the upgrader, so recognising them no longer depends on registry contents.To point an asset at a different ledger, detach it from its accounts, remove it, and create a new one. That is the correct handling regardless: balances do not follow the pointer, so an in-place repoint would leave the station reporting holdings that live on the previous ledger.
Changes
RequestOperation::validateforEditAssetnow callsEnsureAsset::ledger_canister_id_preserved, so a repoint is refused before the request is created rather than after it has been approved. That helper andAssetService::edit()both callAsset::changes_ledger_canister_id, so the two paths cannot drift. The rejection surfaces as a newAssetValidationError::ImmutableField, threaded throughValidationErrorintoRequestErrorandRequestPolicyError. Separately,is_external_canistergained the two pinned system ledger principals.Tests
cargo test -p station --libpasses (401). Clippy andcargo fmtclean.Follow-ups
AddAssetwould remove that dependency entirely. Separate PR, needs stable memory.Note
AssetService::createtakes an optionalwith_asset_id, which is not reachable fromAddAssetOperationInputand is used only by init and migrations. Worth keeping it that way.