fix(station): make asset ledger canister id immutable after creation - #648
Open
MRmarioruci wants to merge 1 commit into
Open
fix(station): make asset ledger canister id immutable after creation#648MRmarioruci wants to merge 1 commit into
MRmarioruci wants to merge 1 commit 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 553a402. Security Overview
Detected Code Changes
|
Contributor
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.
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.
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.AssetService::edit()now rejects an edit that repoints or removes it once set. Setting one that was previously absent is still allowed, since an asset without a ledger cannot transact.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.
Tests
cargo test -p station --libpasses (392). Clippy andcargo fmtclean.Note
AssetService::createtakes an optionalwith_asset_id, which is not reachable fromAddAssetOperationInputand is used only by init and migrations. Worth keeping it that way.