Apply chain updates #40
Conversation
| @@ -0,0 +1,1078 @@ | |||
| export default [ | |||
There was a problem hiding this comment.
Should we add v number to name for disambiguation? eg TokenPool_v1_5.ts. If we need to add compat for v1.4 of applyChainUpdates() it may be clearer by adding v number in the name. WDYT?
| 'generate calldata for applyChainUpdates function, using a JSON file to pass arguments. Useful for working with multisig wallets', | ||
| (yargs) => | ||
| yargs.positional('json_args', { | ||
| type: 'string', |
There was a problem hiding this comment.
Note: I was seeing a TS compiler error here - but it appears to be invalid. If you're not seeing it then it's specific to my env. but if you're seeing it please check its not an issue?
| // ================================================================ | ||
| // │ Validate Calldata │ | ||
| // ================================================================ | ||
| if (calldata !== txRequest.data) { |
There was a problem hiding this comment.
since the args remoteChainSelectorsToRemove and chainsToAdd are the return values from generateApplyChainUpdatesCalldata , and the result of calling encodeFunctionData on the interface, in what circumstances would calldata !== txRequest.data? If there are material situations in this arises, should we provide the possible explanation for the diffs in the readline outputs so that the end user knows whether to choose Y or N based on intelligent understanding? If not are we not just scaring them off/ making them choose without understanding which defeats the point of this check?
| try { | ||
| await poolContract.applyChainUpdates.staticCall(remoteChainSelectorsToRemove, chainsToAdd) | ||
| console.log('✅ Transaction simulation successful! No errors detected.') | ||
| } catch (error) { |
There was a problem hiding this comment.
nit: error:any (give type any to error) to silence ts compiler on use of error in next line
|
Thanks @andrejrakic . Left some comments but subject always to @andrevmatos who is the guru on this repo. |
…converting Solana addresses to bytes32
zeuslawyer
left a comment
zeuslawyer
left a comment
There was a problem hiding this comment.
Left some small comments for your consideration in src/commands/apply-chain-updates.ts
| }, | ||
| "dependencies": { | ||
| "@inquirer/prompts": "7.5.1", | ||
| "@solana/web3.js": "^1.98.2", |
There was a problem hiding this comment.
in the main PR description maybe add an "UPDATE" section and call out that this package may be deprecated and there are other packages involved but due to current dependencies we cannot update them all and maybe add that analyses in a new GH Issue and link to it in the Description?
| */ | ||
| function tryEncodeSolanaAddressToBytes32(address: string): string { | ||
| // Skip processing if it doesn't look like a Solana address | ||
| if (!looksLikeSolanaAddress(address)) { |
There was a problem hiding this comment.
shouldnt this logic be run outside of this function before invoking it? it feels a bit odd that the function is silent if its not a solana address esp since the name is to encode a solana address
| * returns a 32-byte 0x-prefixed hex string; otherwise returns the original input. | ||
| * Only processes addresses that pass the Solana format check to avoid noise. | ||
| */ | ||
| function tryEncodeSolanaAddressToBytes32(address: string): string { |
There was a problem hiding this comment.
is there a type alias for 0xstring that we should used instead of string return type? if not no worries
| const pubkey = new PublicKey(address) | ||
|
|
||
| // Check if the public key is on the ed25519 curve and warn if not | ||
| if (!PublicKey.isOnCurve(pubkey.toBytes())) { |
There was a problem hiding this comment.
curious - if address is not on the curve would it pass the conversion in line 59-63? if it would not then maybe we dont need to double assert?
|
#58 (and #61) are mostly a ground-up rewrite, needed to properly support multiple chain families in the codebase; Also, it's worth noting the new version of getSupportedTokens command: it uses a few generic methods from the chains to interact with the TokenAdminRegistries and TokenPools; anything related to administering the TokenPools can follow suit. I'll leave this open, for reference, but it needs to be re-implemented; but I think it'll be much easier and saner in this new repo architecture. |
3 participants
@zeuslawyer @andrevmatos I am opening this draft PR to add support for calling the
applyChainUpdatesfunction on theTokenPool.solsmart contract, given that some users find arguments for it complex to handle directly from Etherscan or similar tools.I've added so far:
applyChainUpdatesfunctionapplyChainUpdatesfunction only, based on the JSON args. No need for provider or signer objects of any kind, useful for pasting to Gnosis Safe UI for example.This PR currently supports only v1.5 pools. I am opening this draft PR also to comment and validate approach I described. If we think this is the most user friendly approach, then I would like to add support for v1.4 pools as well. The core difference is that v1.4 pools have
applyChainUpdates(ChainUpdate[] calldata chains)function signature with0xdb6327dcselector, while v1.5 pools haveapplyChainUpdates(uint64[] calldata remoteChainSelectorsToRemove, ChainUpdate[] calldata chainsToAdd)function signature with0xe8a1da17selector. AlsoChainUpdatestruct is different between these two versions.What I suggest is to have another JSON args template, and based on the content of the JSON args "detect" whether the user wants to interact with v1.4 or v1.5 pool, instead of providing CLI flag (although I can do that as well). Any objections to this?
And do we want ccip-tools-ts to support the
applyRampUpdatesfunction from v1.2 token pools, which serves a similar purpose?Thank u