fix: Solana batch ops logic in ConfigureTokensForTransfers#2051
fix: Solana batch ops logic in ConfigureTokensForTransfers#2051chris-de-leon-cll wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Solana ConfigureTokenForTransfers batching so AcceptTokenAdminRegistry doesn’t re-read stale Token Admin Registry (TAR) state when RegisterTokenAdminRegistry produced MCMS proposal instructions that won’t be applied on-chain until the batch executes.
Changes:
- Extend
TokenAdminRegistryOutto carry the underlying proposal instruction(s) so callers can construct a single ordered MCMS batch containing both “register/override” and “accept”. - Add
BuildAcceptTokenAdminRegistrySolanaInstructionhelper and updateAcceptTokenAdminRegistryto use it. - Update the Solana
ConfigureTokenForTransfersSequenceto conditionally build a combined MCMS batch, and add an integration test scenario covering the real-world failure mode.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
integration-tests/deployment/token_expansion_scenarios_test.go |
Adds Scenario 6 integration test reproducing third-party TAR pending-admin + cross-family token expansion flow. |
chains/solana/deployment/v1_6_0/sequences/tokens.go |
Updates configure-for-transfers sequence to combine proposal+accept into one MCMS batch when needed. |
chains/solana/deployment/v1_6_0/operations/router/router.go |
Adds proposal-instruction plumbing + accept-instruction builder to support combined batching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
chains/solana/deployment/v1_6_0/operations/router/router.go:612
pendingAdminis guaranteed to be non-zero by the time this condition is evaluated (it is set toinput.Adminin thependingAdmin.IsZero()branch above). This makes thependingAdmin.IsZero()check here redundant and the adjacent comment misleading (it suggests a zero pending admin is possible at this point). Consider simplifying the condition (and updating the comment) to reduce confusion about the accept/proposal decision.
// pending admin unset = we're proposing and accepting in the same batch, so timelock must be the signer
// pending admin set = we need a proposal if the admin is not the deployer
if pendingAdmin.IsZero() || pendingAdmin != chain.DeployerKey.PublicKey() {
batches, err := utils.BuildMCMSBatchOperation(
chain.Selector,
[]solana.Instruction{ixn},
| deployerPK := chain.DeployerKey.PublicKey() | ||
| routerPK := solana.PublicKeyFromBytes(routerAddr) | ||
|
|
||
| // If the proposed token admin is timelock or the deployer, then we can run Register + Accept |
There was a problem hiding this comment.
Can it happen that maybe you need to config the token pool first (for example by running set_pool) and then propose a new administrator with everything already configured?
There was a problem hiding this comment.
SetPool should be called after this during the sequence. I guess the only scenario where we couldn't call SetPool after admin registration is if we were registering to an address CLL didn't control? So that would be the manual registration flow and the customer can set it afterwards?
|
Summary
This PR fixes a bug in the Solana
ConfigureTokenForTransferssequence related to theRegisterTokenAdminRegistryandAcceptTokenAdminRegistrysteps. We illustrate the bug using a real-world example. Suppose:Assume the state of the token admin registry (TAR) on Solana looks like this:
11111111111111111111111111111111With the current code on
main, running token expansion for this scenario leads to the following error:In this case,
RegisterTokenAdminRegistryneeds a proposal and adds anNewCcipAdminOverridePendingAdministratorInstructionto the batch here.Then,
AcceptTokenAdminRegistryruns next and queries RPC before the proposal was applied on-chain, so it sees the stale pending admin and rejects it here.To fix this, we need to conditionally handle the output of
RegisterTokenAdminRegistry:RegisterTokenAdminRegistryrequires a proposal and we're overriding the pending admin with timelock, then we should avoid inspecting the current pending admin in the TAR and schedule an Accept instruction in the same batch.RegisterTokenAdminRegistrydoes not require a proposal, then we can keep the existing flow whereAcceptTokenAdminRegistryruns as a separate operation and inspects the live onchain state of TAR.What changed
TokenAdminRegistryOut: addProposalInstructions []solana.Instruction(paired withBatchOpswhenever register/transfer wrapped a proposalix).BuildAcceptTokenAdminRegistrySolanaInstruction: buildsAcceptAdminRolefor a givenpendingAdministratorpubkey;Acceptreuses this after its RPC/policy checks.ConfigureTokenForTransfers(Solana adapter):ProposalInstructionsandBatchOpspresent →append(proposal ix(s), built accept ix) →BuildMCMSBatchOperationonce (no interimAcceptthat rereads stale TAR for that fork).Acceptas a separateExecuteOperation,BatchOpsfrom register then accept appended.Integration (“Scenario 6”): two
TokenExpansionpasses with explicit third-partyPendingAdministrator, then cross-family pools; asserts success and final TAR:Mintunchanged,Administrator= timelock signer PDA,PendingAdministrator= zero, customer key ≠Administrator.