feat: Solana 1.6 pool support in ConfigureTokenPool - #2218
Conversation
- stop treating any GetAuthority* read failure as "pool not initialized"; reuse the pool config read the op already performed, keeping the upgrade-authority fallback for the account-absent case only - document that Solana callers must pass the pool config PDA as TokenPoolRef, not the pool program ID - drop no-op ,omitempty tags from struct-typed input fields - wrap dropped address-parse errors
|
👋 futureproof1, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
chris-de-leon-cll
left a comment
chris-de-leon-cll
left a comment
There was a problem hiding this comment.
Overall looks good - just had one comment about simplifying the sequence input, so once that's resolved we should be good to merge!
| // if we had to assume the authority, then the pool isn't initialized yet and therefore | ||
| // there won't be an authority set on-chain yet, so we can skip the update since the initializer | ||
| // will be able to set the correct authority during initialization | ||
| if authority == input.NewOwner { | ||
| b.Logger.Info("New owner is the same as the current owner for burn mint token pool with token mint:", input.TokenMint.String()) | ||
| return sequences.OnChainOutput{}, nil | ||
| } |
There was a problem hiding this comment.
[no action needed]
Nice catch here! Looks like this original code was buggy and can be safely removed. It seems like this check was asking: "Is the person who controls the program the same person we want to make the rate limit admin?" If yes, then skip. Don't set anything.
That's incorrect. The operation should ALWAYS set the rate limit admin. Whether the upgrade authority happens to be the same person or a different person is irrelevant.
| // Nil fields are left unchanged on-chain. | ||
| type SetTokenPoolAdminsSequenceInput struct { | ||
| // Selector is the chain selector for the chain on which the pool lives. | ||
| Selector uint64 `json:"selector" yaml:"selector"` |
There was a problem hiding this comment.
Ah looks like PoolAddress is redundant with TokenPoolRef.Address in SetTokenPoolAdminsSequenceInput, so we can safely remove it
Rationale:
- The only production call site (
configure_token_pool.go:185) setsPoolAddress: fullPoolRef.Address— the same value thatTokenPoolRef.Addressalready carries after resolution. - EVM's
SetTokenPoolAdmins(chains/evm/deployment/v1_0_0/adapters/pool_adapter.go:210) readsinput.PoolAddress; switching toinput.TokenPoolRef.Addressis a one-line change. - Solana never reads
input.PoolAddressat all — it usesinput.TokenPoolRef.Typeandinput.TokenRef.Address.
Changes required:
| File | Change |
|---|---|
deployment/tokens/product.go |
Remove PoolAddress from SetTokenPoolAdminsSequenceInput |
chains/evm/deployment/v1_0_0/adapters/pool_adapter.go |
Line 210-213: input.PoolAddress → input.TokenPoolRef.Address |
deployment/tokens/configure_token_pool.go |
Line 187: remove PoolAddress: line |
There was a problem hiding this comment.
PoolAddress is gone and both adapters now read input.TokenPoolRef.Address.
One other thing — Solana did read input.PoolAddress, at four pats in chains/solana/deployment/v1_6_0/sequences/tokens.go: line :640 parsed it into the pool program ID passed to the operation, and line :614,line :642 and line :655. So the removal touched that file too, otherwise it wouldn't compile.
…nput PoolAddress always carried the same value as TokenPoolRef.Address — the single call site set both from fullPoolRef — so the two could drift if a direct caller populated them inconsistently. Both adapters now read the pool address from TokenPoolRef.Address. Addresses review feedback on #2218.
There was a problem hiding this comment.
Pull request overview
Extends the ConfigureTokenPool changeset to support Solana v1.6 BurnMint/LockRelease pools for rateLimitAdmin updates, while explicitly rejecting unsupported admin fields, and adds Solana integration coverage for both admin updates and the legacy fee-config (FeeQuoter) path.
Changes:
- Widened
SetTokenPoolAdminsSequenceInputto carry resolvedTokenPoolRef+TokenRef, and updatedConfigureTokenPool(and the EVM adapter) to useTokenPoolRef.Addressinstead of the removedPoolAddress. - Implemented
SetTokenPoolAdmins()for Solana v1.6 and fixed Solana rate-limit-admin operations to properly distinguishrpc.ErrNotFoundfrom other read errors and to perform correct idempotency checks. - Added integration tests covering Solana admin updates, unsupported-field rejection, and fee-config application/readback for Solana pools (FeeQuoter path).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/deployment/configure_token_pool_test.go | Adds Solana integration helpers/tests for rateLimitAdmin updates, unsupported field rejection, and fee config readback. |
| deployment/tokens/product.go | Updates admin sequence input to include resolved TokenPoolRef and TokenRef. |
| deployment/tokens/configure_token_pool.go | Documents Solana TokenPoolRef requirements and passes resolved refs into SetTokenPoolAdmins. |
| chains/solana/deployment/v1_6_0/sequences/tokens.go | Adds Solana v1.6 SetTokenPoolAdmins() implementation (rateLimitAdmin support; feeAdmin rejection). |
| chains/solana/deployment/v1_6_0/operations/token_pools/lockrelease.go | Fixes error handling/idempotency logic in Solana LnR UpdateRateLimitAdmin. |
| chains/solana/deployment/v1_6_0/operations/token_pools/burnmint.go | Fixes error handling/idempotency logic in Solana BnM UpdateRateLimitAdmin. |
| chains/evm/deployment/v1_0_0/adapters/pool_adapter.go | Switches EVM admin sequence to read the pool address from input.TokenPoolRef.Address. |
Suppressed comments (1)
integration-tests/deployment/configure_token_pool_test.go:851
- The inline comment "helper from Step 6" appears to refer to PR/review steps rather than something in the codebase, which makes it confusing and likely to go stale. Suggest removing it.
env, bnm, lnr := setupSolanaPoolsForConfigure(t) // helper from Step 6
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
What
Makes
ConfigureTokenPool'srateLimitAdminfield work for Solana 1.6 BurnMint and LockRelease token pools, and adds integration coverage proving the unsupported fields reject loudly and thetokenTransferFeeConfigpath works for Solana.Follows #2211, which did the equivalent EVM work.
SolanaAdapterjoins the existing chain-agnosticTokenPoolAdminAdapterseam by type assertion — no new interface. Because a Solana pool address is a program ID shared across many token mints,SetTokenPoolAdminsSequenceInputgainsTokenPoolRefandTokenRef, which the changeset already resolved and previously discarded. Both adapters read the pool address fromTokenPoolRef.Address—PoolAddresswas redundant and dropped in commit 5 (see below). The EVM adapter changes are trivial:input.PoolAddress→input.TokenPoolRef.Address.Commits
fix: correct idempotency and error handling in Solana rate limit admin ops— two defects inUpdateRateLimitAdmin{BurnMint,LockRelease}, which are already live in the Solana pool deploy path:if err == nil { compare }made a transient RPC or decode failure indistinguishable from "account absent", and both fell through to writing — degrading the no-op guarantee to "always write". Now onlyrpc.ErrNotFoundfalls through; every other error is returned.if authority == input.NewOwner { return nil }compared the program upgrade authority against the desired rate limit admin. When the deployer key was both — normal in tests and single-operator deployments — the op reported success having done nothing.feat: support rateLimitAdmin on Solana 1.6 token pools in ConfigureTokenPool— widens the sequence input, passes the already-resolved refs at the call site, and implementsSetTokenPoolAdmins()onSolanaAdapter.feeAdminis rejected (Solana pools have no fee admin concept); a nilrateLimitAdminis a no-op.test: cover Solana rejection paths and fee config in ConfigureTokenPool—feeAdminandfinalityConfigrejection, plus thetokenTransferFeeConfigpath, which routes Solana pools to the FeeQuoter because their version is below 2.0.0. That path was believed to work but nothing verified it; it does, and the test reads the config back on-chain through the same resolution path the changeset uses.fix: address review findings on the Solana rate limit admin path— see below.refactor: drop redundant PoolAddress from SetTokenPoolAdminsSequenceInput—PoolAddressalways carried the same value asTokenPoolRef.Address. Removed. EVM adapter switched frominput.PoolAddresstoinput.TokenPoolRef.Address.Two things reviewers should know
Solana callers must pass the pool config PDA as
TokenPoolRef, not the pool program ID. The genericResolveTokenPoolRefdoes an exact-match datastore lookup by address before the family-specific resolver runs, and a bare Solana pool program ID is exactly what the datastore registers — so the lookup short-circuits and the Solana PDA-normalizing resolver never runs, and token derivation then fails with "token derivation is only possible if a pool PDA is provided". This is inherent rather than a workaround: a program ID is shared across mints and so cannot identify a token. The requirement is now documented onPoolConfigUpdate.TokenPoolRef, where it previously lived only in a test comment.Widening
SetTokenPoolAdminsSequenceInputchanges its report hash. Any durable or resumable report store will no longer match previously-recordedSetTokenPoolAdminsreports and will re-execute them. That is safe — the operation reads on-chain state and no-ops before writing — but it is worth knowing.Review fixes folded into commit 4
GetAuthority*failure, not just "account absent", fell back to the program upgrade authority, so a transient read failure could submitSetRateLimitAdminsigned by the wrong authority. Now reuses the pool config read the op already performed — which also removes a redundant RPC round-trip per pool — and keeps the upgrade-authority fallback for the account-absent case only. That fallback is load-bearing: under MCMS the initialize op only queues a proposal, so the pool config account genuinely does not exist yet when this op runs.ExecuteSequence's report memoization returned the first apply's cached report without running the handler — the read-compare no-op path was never exercised. Now refreshes the reporter first, matching the idiom every pre-existing EVM test in that file already uses.,omitemptytags from the two struct-typed input fields (encoding/jsonnever omits struct values), wrapped dropped address-parse errors with%w, and asserted the third fee-config value (MinFeeUSDCents) actually landed on-chain.Deliberately not changed
UpdateRateLimitAdmin{BurnMint,LockRelease}keep their signatures — the input type is shared with the transfer-ownership and accept-ownership ops, and these are exported vars in a public module, so a new input type would be source-breaking downstream. That is whyNewOwnercarries the desired rate limit admin, which reads oddly but is intended. The ops also keep decoding pool state astest_token_pool.State, which has the same wire layout.Testing
TestConfigureTokenPool— 12 subtests pass, including every pre-existing EVM testTestTokensAndTokenPools— passes; this is the regression harness for the operation changes, and it deploys Solana BurnMint and LockRelease pools through MCMSgo buildandgo vetclean inchains/solana,chains/solana/deployment,deployment,integration-tests, andchains/evmFollow-ups, not blockers
feeAdminrejection, nil-admin no-op, unknown selector) all run before the chain lookup and are trivially unit-testable without a Solana container.TestConfigureTokenPool_FeeConfig_SolanaandTestConfigureTokenPool_UnsupportedFields_Solanaeach stand up their own Solana container; they are read-mostly and could share one environment.