Glamsterdam changeset - #2205
Conversation
| continue | ||
| } | ||
|
|
||
| result := glamsterdamutils.Resolve(USDCTokenPoolDestGasOverhead, cur.Output.DestGasOverhead) |
There was a problem hiding this comment.
is the intention to 3x the overhead gas for every token with explicit override? if so this makes sense, but maybe change the name a bit? Resolve(USDCTokenPoolDestGasOverhead in a global loop is a bit confusing
matYang
left a comment
matYang
left a comment
There was a problem hiding this comment.
Found four correctness issues in retry handling and pool selection/update coverage.
Minor: AddUnresolvedContract is used for a missing optional CommitteeVerifier, so the proposal description says the chain was skipped even though its OnRamp, FeeQuoter, and pool updates still proceed.
| if current == spec.ExpectedPrague { | ||
| return FieldResult[T]{Spec: spec, Current: current, Matched: true, AppliedValue: spec.GlamsterdamValue} | ||
| } | ||
| return FieldResult[T]{Spec: spec, Current: current, Matched: false, AppliedValue: spec.Fallback(current)} |
There was a problem hiding this comment.
On a rerun after this proposal executes, current is already the applied Glamsterdam value, so it enters this mismatch branch and gets multiplied again. For example, v1.6 DestGasOverhead 500,000 becomes 833,333 and v2 BaseExecutionGasCost 400,000 becomes 800,000. That makes overlapping or partial retries unsafe. Treat already-applied values as no-ops, and make the desired result explicit for custom-baseline fallback values so they cannot compound either.
There was a problem hiding this comment.
This is tricky but a very good point. On a run how can the changeset know if a value has already been multiplied? And even if the changeset does check and see "hey this value already matches what is needed for glamsterdam" how does it know that really that wasn't some pre-set custom value and to actually make it glamsterdam safe it should be multiplied by some factor...
I mean we could do something where the changeset distinguishes between expected values for some chain family, and if a chain is currently using those expected values then it applies the default glamsterdam change to them. But if not then the user has to specify what the new value should be for that chain. So there is no automatic multiply if the gas values aren't matching defaults used.
| continue | ||
| } | ||
|
|
||
| result := glamsterdamutils.Resolve(USDCTokenPoolDestGasOverhead, cur.Output.DestGasOverhead) |
There was a problem hiding this comment.
The runbook scopes this row to USDC lanes, but CandidateTokens comes from getAllConfiguredTokens, so it contains every TokenAdminRegistry token. Any non-USDC token with an enabled custom override reaches this line and gets the USDC-specific 3x ratio. Resolve the chain's USDC token and only update that token instead of rewriting every enabled override.
| } | ||
| lanes = append(lanes, lane) | ||
|
|
||
| if lombardRef := datastore_utils.GetAddressRef(addrs, sel, lombard_token_pool.ContractType, lombard_token_pool.Version, ""); !datastore_utils.IsAddressRefEmpty(lombardRef) { |
There was a problem hiding this comment.
Pool discovery is incomplete here: GetAddressRef returns only the first Lombard pool even though Lombard deployment supports multiple qualifier-scoped pools per chain, and the USDC lookup below only includes SiloedUSDCTokenPool, omitting the CCTPThroughCCVTokenPool configured for CCTP-capable EVM remotes. Those omitted pools retain their Prague DestGasOverhead. Enumerate all matching Lombard refs and every USDC pool implementation that owns a target-lane config.
| ) | ||
| } | ||
|
|
||
| result := glamsterdamutils.Resolve(spec, cur.Output.DestGasOverhead) |
There was a problem hiding this comment.
GetTokenTransferFeeConfig returns the zero struct when this pool has no config for the target lane. This code preserves IsEnabled=false and builds a write, but TokenPool.applyTokenTransferFeeConfigUpdates rejects disabled configs and unsupported chains, so the MCMS batch reverts when executed. Skip pools whose target config is not enabled/supported before resolving and encoding the update.
|
3 participants
No description provided.