Skip to content

chore: migrate motoko/icrc2-swap to icp-cli#1386

Merged
marc0olo merged 18 commits into
masterfrom
chore/migrate-icrc2-swap-to-icp-cli
Jun 16, 2026
Merged

chore: migrate motoko/icrc2-swap to icp-cli#1386
marc0olo merged 18 commits into
masterfrom
chore/migrate-icrc2-swap-to-icp-cli

Conversation

@marc0olo

@marc0olo marc0olo commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

Migrates motoko/icrc2-swap from dfx to icp-cli.

Originally contributed by 0xAegir.

Code:

  • backend/app.mo: converted from actor class Swap(init_args) to plain actor Swap; token principals read from PUBLIC_CANISTER_ID:token_a / PUBLIC_CANISTER_ID:token_b via Runtime.envVar<system> — icp-cli injects them during deploy; no init args needed
  • Security improvements (from canister-security skill review):
    • withdraw: added try/catch around icrc1_transfer — if the token ledger traps, the balance is refunded and #CallFailed returned; previously the balance was permanently debited
    • withdraw: reject anonymous principal (uses msg.caller for balance lookup)
    • swap: check both users have non-zero balances before swapping; returns #InsufficientBalance instead of silently moving zero tokens
  • ICRC.mo: inline ICRC-1/2 type definitions kept for educational visibility

Deploy (make deploy):

  • Creates icrc2-alice / icrc2-bob identities with --storage plaintext (compatible with CI containers that have no system keyring)
  • Deploys token_a pre-funded for icrc2-alice, token_b pre-funded for icrc2-bob
  • Deploys backend with --mode reinstall -y (works on fresh and existing deployments)
  • No --args needed for backend — it discovers token principals via env vars

Tests (make test, 7 tests, idempotent):

  1. Empty balances on fresh deployment
  2. Swap with no deposits returns InsufficientBalance
  3. Alice approves + deposits token_a (approve = amount + fee)
  4. Bob approves + deposits token_b
  5. Swap 1:1
  6. Alice withdraws token_b; verifies ledger balance delta = 99_990_000
  7. Bob withdraws token_a; verifies ledger balance delta = 99_990_000

README:

  • Leads with the two key safety patterns (debit-before-transfer, atomic swap)
  • Fee handling section explaining approve/withdraw arithmetic
  • #CallFailed guidance: retry; if persistent, contact token ledger operator
  • Known limitations accurately describe async edge cases
  • Links to ICRC-2 digital asset standards and inter-canister calls security best practices

Test plan

  • mops check passes
  • CI motoko-icrc2-swap job passes all 7 tests

marc0olo and others added 18 commits June 12, 2026 22:14
Replace dfx.json with icp.yaml, migrate Motoko sources from src/swap/ to
backend/, add icp-dev-env-motoko:0.3.2 CI workflow, update mops.toml to
moc 1.9.0 / core 2.5.0 with --default-persistent-actors. Remove JS/Jest
test suite, old deploy.sh, and prettier config; add Makefile with deploy
and test targets that handle the multi-step deployment (token_a, token_b
ICRC-1 ledger canisters via pre-built WASM, then backend swap canister
with dynamic init args containing the token canister IDs).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… README

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Deploy:
- --argument → --args (correct icp-cli flag)
- icp canister id → icp canister status X -i

Tests:
- Replace single-user contrived test with real two-user scenario:
  Alice deposits token_a, Bob deposits token_b, they swap 1:1,
  each withdraws the other's token
- Use --identity test-alice/test-bob for two distinct callers

README:
- Credit original author 0xAegir (AegirFinance)
- Remove "DeFi is experimental" disclaimer
- Fix security link: old URL → docs.internetcomputer.org/guides/security/inter-canister-calls
- Note that make deploy must be used instead of icp deploy
- Explain what make test actually tests

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…quired

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…init args

- Change from actor class Swap(init_args) to plain actor Swap
- Token principals read from PUBLIC_CANISTER_ID:token_a / token_b via
  Runtime.envVar<system>() — injected automatically by icp-cli during deploy
- Remove --args from make deploy backend step; no init args needed
- Backend deploys last so icp-cli has both token IDs to inject
- Makefile deploy and test use correct icp-cli flags (--args, status -i)
- M0155 (Nat subtraction may trap) intentionally left as a warning —
  subtraction is guarded by explicit balance check but Motoko's type
  checker lacks path-sensitive refinement

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Create icrc2-alice and icrc2-bob identities in make deploy (idempotent)
- token_a initial_balances → icrc2-alice; token_b → icrc2-bob
- No funding/minting step needed in make test — identities start with tokens
- Tests use --identity icrc2-alice/icrc2-bob for realistic two-user flow
- README updated to explain the named identities and env var discovery

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
$(eval ALICE := $(shell ...)) in Make recipe bodies expands before any
recipe commands run — so icp identity principal ran before icp identity new.
Fix: use a single shell block with set -e so identity creation and principal
lookup happen in the correct order within one shell process.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
.gitignore: remove stale token_a.args/token_b.args/backend.args entries
  from the old dfx arg-file approach

Makefile:
- Tests 5/6 use single shell block to capture balance delta (before/after)
  making them idempotent across multiple make test runs
- Fee arithmetic comments added to approve/withdraw steps

README:
- Lead with the two key safety patterns (debit-before-transfer, atomic swap)
  which are the core educational content — previously buried in code comments
- Add ic-mops to prerequisites
- Explain ICRC.mo inline type definitions
- Add fee handling section explaining the approve/withdraw arithmetic
- Clarify make deploy vs icp deploy and why
- Tighten known limitations section

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ejection

From canister-security skill review:

Pitfall #11 (callback trap after state mutation):
- withdraw: balance is debited before await icrc1_transfer
- if the token ledger TRAPS (not just returns an error), the callback
  never ran and the refund in the #Err branch is skipped
- Fix: wrap await icrc1_transfer in try/catch; on trap, refund and
  return #CallFailed with the error message
- Add #CallFailed error variant to WithdrawError

Pitfall #2 (anonymous principal):
- withdraw uses msg.caller as balance key but never rejected anonymous
- Fix: trap on anonymous caller at entry

README known limitations updated to accurately describe:
- the async edge case for trusted-only token canisters (not "deadlock")
- the deposit ambiguous-outcome scenario

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tor escalation

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
swap() now returns InsufficientBalance if either user has no deposit —
previously it silently succeeded moving zero tokens.

Add Test 2 (before any deposits) to verify InsufficientBalance fires,
making the guard visible in the test output.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The default --storage keyring requires a system keyring unavailable in
CI containers. --storage plaintext stores the key as a plain PEM file,
which works in both CI and local dev.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@marc0olo marc0olo marked this pull request as ready for review June 16, 2026 10:08
@marc0olo marc0olo requested review from a team as code owners June 16, 2026 10:08
@marc0olo marc0olo merged commit 09af6a8 into master Jun 16, 2026
6 checks passed
@marc0olo marc0olo deleted the chore/migrate-icrc2-swap-to-icp-cli branch June 16, 2026 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants