feat: tooling for ICAs with "DAO ISM"s #4773
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4773 +/- ##
=======================================
Coverage 74.27% 74.27%
=======================================
Files 101 101
Lines 1481 1481
Branches 192 192
=======================================
Hits 1100 1100
Misses 360 360
Partials 21 21
|
tkporter
requested review from
paulbalaji,
Mo-Hussain,
yorhodes and
jmrossy
as code owners
| // Found by running: | ||
| // yarn tsx ./scripts/get-owner-ica.ts -e mainnet3 --ownerChain ethereum --destinationChains <chain1> <chain2> ... | ||
| export const icas: Partial< | ||
| export const oldIcas: Partial< |
There was a problem hiding this comment.
maybe move these to a legacy-aw-icas.json with zero address for ism, so we have a consistent way of reading/writing/persisting ica information
| const actualAccount = await ica.getAccount(chain, { | ||
| ...ownerConfig, | ||
| ismOverride: expected.ism, | ||
| routerOverride: ownerChainInterchainAccountRouter, | ||
| }); | ||
| if (!eqAddress(expected.ica, actualAccount)) { |
There was a problem hiding this comment.
wonder if there's anything we can do to dedupe between here + get-owner-ica, so we don't accidentally check one thing and deploy another
| Expected: expected, | ||
| Actual: { | ||
| ica: actualAccount, | ||
| ism: expected.ism, | ||
| }, |
There was a problem hiding this comment.
yeah moving old icas to a legacy-aw-ica could help us double check those like this too
should we also include the router address in here for reproducible checks?
| } | ||
| } | ||
|
|
||
| // -- ISM config generation -- |
There was a problem hiding this comment.
all of this config generation should live at infra/config/ not infra/scripts
probably could split out the owners.ts a little so we have the "aw-ica" stuff there
| // Ensure that we can derive the ICA using the ISM we're aware of. | ||
| const ismOverride = | ||
| this.checker.configMap[chain].ownerOverrides?._icaIsmAddress; | ||
| const recoveredAccount = await this.interchainAccount.getAccount(chain, { | ||
| owner: accountConfig.owner, | ||
| origin: accountConfig.origin, | ||
| ismOverride, | ||
| }); | ||
| if (!eqAddress(recoveredAccount, account.address)) { | ||
| console.error( | ||
| chalk.red( | ||
| `Failed to recover the target owner ICA: (chain: ${chain}, remote owner: ${accountConfig.owner}, origin: ${accountConfig}). | ||
| Used ISM override: ${ismOverride}, recovered account: ${recoveredAccount}. Is the ICA's ISM override correct? | ||
| Defaulting to manual submission.`, | ||
| ), | ||
| ); | ||
| return { | ||
| type: SubmissionType.MANUAL, | ||
| chain, | ||
| call, | ||
| }; | ||
| } |
There was a problem hiding this comment.
so once we have deployed the new icas and uncomment out the relevant code for them in the config, i'm not sure I see how we still submit to the old icas to transfer ownership to the new ones
There was a problem hiding this comment.
Maybe I'm misunderstanding but I thought transferring would involve:
- changing the owner to the new ICA
- we keep
_icaAddressas the old ICA, so we're capable of sending a tx via the old ICA to transfer to the new owner
And then after that's executed, we:
- update
_icaAddressto be the new ICA and_icaIsmAddressas needed, so we can submit new txs via this new ICA
There was a problem hiding this comment.
ahhh yes - because we update the owner to be artifact ?? safe ?? deployer but don't change the actual ica that things get submitted through the first time
thanks for reminding me
| // ...(icaArtifact && { _icaAddress: icaArtifact.ica }), | ||
| // ...(icaArtifact?.ism && { _icaIsmAddress: icaArtifact.ism }), |
There was a problem hiding this comment.
should we throw if icaArtifact.ism isn't set?
Description
Drive-by changes
Related issues
Backward compatibility
Testing