Skip to content

feat(cct-sdk): Add transfer admin solana op - #313

Open
mervin-link wants to merge 5 commits into
feat/DAPP-10603-register-tokenfrom
feat/DAPP-10604-propose-admin
Open

feat(cct-sdk): Add transfer admin solana op#313
mervin-link wants to merge 5 commits into
feat/DAPP-10603-register-tokenfrom
feat/DAPP-10604-propose-admin

Conversation

@mervin-link

@mervin-link mervin-link commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

What

  • DAPP-10604
  • Add Solana CCT transferAdmin and generateUnsignedTransferAdmin TokenAdminRegistry operations
  • Validate current admin authority before building transferAdminRoleTokenAdminRegistry

Why

  • Enable SDK consumers to propose a new Solana TokenAdminRegistry administrator

@mervin-link
mervin-link requested a review from apedrob July 27, 2026 17:37
@mervin-link
mervin-link requested review from a team, PabloMansanet and aelmanaa as code owners July 27, 2026 17:37
@vercel

vercel Bot commented Jul 27, 2026

Copy link
Copy Markdown

You must have Developer access to commit code to Chainlink Labs on Vercel. If you contact an administrator and receive Developer access, commit again to see your changes.

Learn more: https://vercel.com/docs/accounts/team-members-and-roles/access-roles#team-level-roles

@aelmanaa aelmanaa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran this op live on Solana devnet:

  • Executed proposeAdmin end-to-end: tx 5sQaftoHuU3cWekpZUri6kqhZVuYsLBYZZTKLjyTFfgpmQYqwxXffSmJ9VRaT9he2xGjWjATY2YYgbpbtDpgCH8m. Read the registry PDA back two independent ways (SDK decode + raw getAccountInfo borsh-offset decode): pending_administrator == the proposed newAdmin, administrator unchanged. Accounts and data match the canonical ModifyTokenAdminRegistry context byte-for-byte.
  • The client preflight mirrors the on-chain constraint (authority == administrator @ Unauthorized, token_context.rs:132) exactly — neither stricter nor looser — and both negative paths reject client-side with typed errors before any transaction (wrong-state authority; authority ≠ executing wallet). Nice.
  • Type discipline, error taxonomy, layer boundaries, and the facade/barrel pattern all match the sibling ops; 4/4 new tests pass at head.

Two things I'd ask for before merge, both raised inline: the rename to transferAdminRole (the chain's own program log for this op's transaction is Instruction: TransferAdminRoleTokenAdminRegistry; "propose" is the registration verb everywhere else in CCIP — including our own bs58 generator), and the journey docs hook-up once DAPP-10605's accept op exists. On that second point, an operational note: to test this op at all I had to perform the accept instruction outside the SDK — after registerToken the registry's administrator is the zero default, so proposeAdmin/transferAdminRole has nothing it can act on until the accept op lands. The two PRs are really one journey; landing 10604 → 10605 in order (10605 rebased onto this head) and cross-linking their docs would close the register-token review's F1 completely.

(Not this PR's diff, but noting for the stack: register-token.test.ts:8 on the base branch has an import-x/order warning — errors.ts import above the ../../../../ group; a lint:fix on whichever branch lands first clears it.)

Comment thread ccip-sdk/src/cct/solana/index.ts Outdated
Comment thread ccip-sdk/src/cct/solana/index.ts Outdated
Comment thread ccip-sdk/src/cct/solana/token-admin-registry/operations/propose-admin.test.ts Outdated
Comment thread ccip-sdk/src/cct/solana/token-admin-registry/operations/propose-admin.test.ts Outdated
Comment thread ccip-sdk/src/cct/solana/token-admin-registry/operations/propose-admin.test.ts Outdated
@mervin-link mervin-link changed the title feat(cct-sdk): Add propose admin solana op feat(cct-sdk): Add transfer admin solana op Jul 29, 2026
@mervin-link
mervin-link requested a review from aelmanaa July 29, 2026 10:04

@aelmanaa aelmanaa left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approve — the rename landed correctly and completely; all prior blockers and nits resolved.

Re-reviewed at 4771aef (was dd03030). Every finding from the prior pass is fixed:

  • P1 (rename) ✅transferAdmin end-to-end: class TransferAdmin, facade generateUnsignedTransferAdmin/transferAdmin, op file transfer-admin.ts, tests. No residual proposeAdmin identifier in SDK code. Still wraps the unchanged transferAdminRoleTokenAdminRegistry instruction. Fresh devnet run under the new name is wire-identical (same discriminator b262cbb5cb6b6a0e, same on-chain log Instruction: TransferAdminRoleTokenAdminRegistry).
  • Comments 2–7 ✅ — journey docs wired (@remarks/@see to acceptAdmin + register↔accept↔setPool), test typing (Partial<GenerateTransferAdminParams>), discriminator + account-metas asserts, execute-path reason assert, address docstring trimmed, state-blind message branched (pending-registration case).
  • Import-order lint ✅errors.ts now inside the internal group; eslint clean.

Live re-verification (devnet, 2026-07-29): fresh mint → registerAdmin (owner) → raw accept → transferAdmin executed — on-chain pending_administrator == newAdmin, administrator unchanged, read back via SDK + raw borsh decode. Wire byte-identical to the pre-rename dd03030 run.

One non-blocking nit: the {@link acceptAdmin} / {@link generateUnsignedAcceptAdmin} links are forward references — the accept op ships on DAPP-10605 (PR #317), so they resolve only once #313#317 land in the agreed order.

CI note: npm ci fails on the lockfile sync issue (@emnapi/core@1.11.2 missing) — same repo-wide infra issue as #310/#317, not from this PR's code. Local npm run check is green with a clean install (prettier + eslint + tsc + 5/5 + 9/9 tests).

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