-
Notifications
You must be signed in to change notification settings - Fork 363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create CLI Submitter #3730
Create CLI Submitter #3730
Changes from 4 commits
038966b
4849c80
6865bd1
7d25842
8e4ff93
3d204af
c424ca3
5fdaf9a
2806734
5aee461
1f40e36
9bb15ce
8296880
de28885
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@hyperlane-xyz/sdk': patch | ||
--- | ||
|
||
Exports submitter and transformer props types. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@hyperlane-xyz/cli': minor | ||
--- | ||
|
||
Add CLI-side submitter to use SDK submitter from CRUD and other command modules. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
import { | ||
EV5GnosisSafeTxSubmitter, | ||
EV5ImpersonatedAccountTxSubmitter, | ||
EV5InterchainAccountTxTransformer, | ||
EV5JsonRpcTxSubmitter, | ||
MultiProvider, | ||
TxSubmitterBuilder, | ||
TxSubmitterInterface, | ||
TxSubmitterType, | ||
TxTransformerInterface, | ||
TxTransformerType, | ||
} from '@hyperlane-xyz/sdk'; | ||
import { ProtocolType } from '@hyperlane-xyz/utils'; | ||
|
||
import { forkNetworkToMultiProvider, verifyAnvil } from '../deploy/dry-run.js'; | ||
|
||
import { | ||
SubmitterBuilderSettings, | ||
SubmitterMetadata, | ||
TransformerMetadata, | ||
} from './submitTypes.js'; | ||
|
||
export async function getSubmitterBuilder<TProtocol extends ProtocolType>({ | ||
submitterMetadata, | ||
transformersMetadata, | ||
multiProvider, | ||
}: SubmitterBuilderSettings): Promise<TxSubmitterBuilder<TProtocol>> { | ||
const submitter = await getSubmitter<TProtocol>( | ||
multiProvider, | ||
submitterMetadata, | ||
); | ||
const transformers = await getTransformers<TProtocol>( | ||
multiProvider, | ||
transformersMetadata, | ||
); | ||
|
||
return new TxSubmitterBuilder<TProtocol>(submitter, transformers); | ||
} | ||
|
||
async function getSubmitter<TProtocol extends ProtocolType>( | ||
multiProvider: MultiProvider, | ||
{ type, settings }: SubmitterMetadata, | ||
): Promise<TxSubmitterInterface<TProtocol>> { | ||
switch (type) { | ||
case TxSubmitterType.JSON_RPC: | ||
return new EV5JsonRpcTxSubmitter(multiProvider); | ||
case TxSubmitterType.IMPERSONATED_ACCOUNT: | ||
if (!settings) | ||
throw new Error( | ||
'Must provide EV5ImpersonatedAccountTxSubmitterProps for impersonated account submitter.', | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check out the little There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For whatever reason it seems like we were using this convention more CLI-side, and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the one downside is its slightly less readable to the average TS dev but I like the brevity :) |
||
|
||
await verifyAnvil(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this switch case ever get invoked for a situation other than --dry-run? If not, do we need this call when we already do it in context.ts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're absolutely right– once utilized by any of the command modules this will sit behind the context so there's no need to verify or fork here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont think so |
||
await forkNetworkToMultiProvider( | ||
multiProvider, | ||
settings.eV5ImpersonatedAccountProps.chain, | ||
); | ||
|
||
return new EV5ImpersonatedAccountTxSubmitter( | ||
multiProvider, | ||
settings.eV5ImpersonatedAccountProps, | ||
); | ||
case TxSubmitterType.GNOSIS_SAFE: | ||
if (!settings) | ||
throw new Error( | ||
'Must provide EV5GnosisSafeTxSubmitterProps for Gnosis safe submitter.', | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we have the required There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could also use zod schemas here to validate the input |
||
return new EV5GnosisSafeTxSubmitter( | ||
multiProvider, | ||
settings.eV5GnosisSafeProps, | ||
); | ||
default: | ||
throw new Error(`Invalid TxSubmitterType: ${type}`); | ||
} | ||
} | ||
|
||
async function getTransformers<TProtocol extends ProtocolType>( | ||
multiProvider: MultiProvider, | ||
metadata: TransformerMetadata[], | ||
): Promise<TxTransformerInterface<TProtocol>[]> { | ||
return Promise.all( | ||
metadata.map(({ type, settings }) => | ||
getTransformer<TProtocol>(multiProvider, { type, settings }), | ||
), | ||
); | ||
} | ||
|
||
async function getTransformer<TProtocol extends ProtocolType>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these do not need to be async There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this doesn't look resolved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We chatted about this in the office– since getTransformer must call getCallRemote (which in turn might make a call to network), we're ok with keeping this async There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the async transform doesnt happen in this function iiuc |
||
multiProvider: MultiProvider, | ||
{ type, settings }: TransformerMetadata, | ||
): Promise<TxTransformerInterface<TProtocol>> { | ||
switch (type) { | ||
case TxTransformerType.ICA: | ||
if (!settings) | ||
throw new Error( | ||
'Must provide EV5InterchainAccountTxTransformerProps for ICA transformer.', | ||
); | ||
return new EV5InterchainAccountTxTransformer( | ||
multiProvider, | ||
settings.eV5InterchainAccountProps, | ||
); | ||
default: | ||
throw new Error(`Invalid TxTransformerType: ${type}`); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,30 @@ | ||||||||||||||||||
import type { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Convention is to call this file just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah– I thought I refactored this back to types. Updated |
||||||||||||||||||
EV5GnosisSafeTxSubmitterProps, | ||||||||||||||||||
EV5ImpersonatedAccountTxSubmitterProps, | ||||||||||||||||||
EV5InterchainAccountTxTransformerProps, | ||||||||||||||||||
MultiProvider, | ||||||||||||||||||
TxSubmitterType, | ||||||||||||||||||
TxTransformerType, | ||||||||||||||||||
} from '@hyperlane-xyz/sdk'; | ||||||||||||||||||
|
||||||||||||||||||
export interface SubmitterBuilderSettings { | ||||||||||||||||||
submitterMetadata: SubmitterMetadata; | ||||||||||||||||||
transformersMetadata: TransformerMetadata[]; | ||||||||||||||||||
multiProvider: MultiProvider; | ||||||||||||||||||
} | ||||||||||||||||||
export interface SubmitterMetadata { | ||||||||||||||||||
type: TxSubmitterType; | ||||||||||||||||||
settings?: SubmitterSettings; | ||||||||||||||||||
} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can the settings type be derived from the TxSubmitterType?
Suggested change
something like this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and at that point submitter metadata seems superfluous with the submitter settings interface itself |
||||||||||||||||||
export interface TransformerMetadata { | ||||||||||||||||||
type: TxTransformerType; | ||||||||||||||||||
settings?: TransformerSettings; | ||||||||||||||||||
} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto here |
||||||||||||||||||
|
||||||||||||||||||
interface SubmitterSettings { | ||||||||||||||||||
eV5GnosisSafeProps: EV5GnosisSafeTxSubmitterProps; | ||||||||||||||||||
eV5ImpersonatedAccountProps: EV5ImpersonatedAccountTxSubmitterProps; | ||||||||||||||||||
} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought this would be a union type |
||||||||||||||||||
interface TransformerSettings { | ||||||||||||||||||
eV5InterchainAccountProps: EV5InterchainAccountTxTransformerProps; | ||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,19 @@ | ||
import { Address } from '@hyperlane-xyz/utils'; | ||
|
||
import { ChainName } from '../../../types.js'; | ||
|
||
export enum TxSubmitterType { | ||
JSON_RPC = 'JSON RPC', | ||
IMPERSONATED_ACCOUNT = 'Impersonated Account', | ||
GNOSIS_SAFE = 'Gnosis Safe', | ||
} | ||
|
||
export interface EV5GnosisSafeTxSubmitterProps { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for my own understanding, is this actually specific to ethers v5? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes– on Solana, for example, we may use Snowflake. All the submitter+transformer implementations are protocol-specific (organized by protocol dir, i.e. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reread and I see you may just be referring to this props interface (and not the submitter)– IMO it might be a little early to try and assume an all-inclusive interface, but if you believe something like FWIW we chatted offline on this, and @tkporter mentioned "We use Squads for our Solana deployment (just v2 for now, but v3 soonish). For the transaction submitter rn imo we should just worry about implementing EVM transaction submitters for now and we can deal with Solana if/when we move over to using the CLI for managing Solana deployments" (src) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know gnosis is ethereum specific |
||
chain: ChainName; | ||
safeAddress: Address; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think so, since With that said, I updated the below field on |
||
} | ||
|
||
export interface EV5ImpersonatedAccountTxSubmitterProps { | ||
chain: ChainName; | ||
address: Address; | ||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -3,19 +3,16 @@ import { PopulatedTransaction } from 'ethers'; | |||||||
import { Logger } from 'pino'; | ||||||||
|
||||||||
import { rootLogger } from '@hyperlane-xyz/utils'; | ||||||||
import { Address } from '@hyperlane-xyz/utils'; | ||||||||
|
||||||||
import { ChainName } from '../../../../types.js'; | ||||||||
import { impersonateAccount } from '../../../../utils/fork.js'; | ||||||||
import { MultiProvider } from '../../../MultiProvider.js'; | ||||||||
import { TxSubmitterType } from '../TxSubmitterTypes.js'; | ||||||||
import { | ||||||||
EV5ImpersonatedAccountTxSubmitterProps, | ||||||||
TxSubmitterType, | ||||||||
} from '../TxSubmitterTypes.js'; | ||||||||
|
||||||||
import { EV5JsonRpcTxSubmitter } from './EV5JsonRpcTxSubmitter.js'; | ||||||||
|
||||||||
interface EV5ImpersonatedAccountTxSubmitterProps { | ||||||||
address: Address; | ||||||||
} | ||||||||
|
||||||||
export class EV5ImpersonatedAccountTxSubmitter extends EV5JsonRpcTxSubmitter { | ||||||||
public readonly txSubmitterType: TxSubmitterType = | ||||||||
TxSubmitterType.IMPERSONATED_ACCOUNT; | ||||||||
|
@@ -26,18 +23,17 @@ export class EV5ImpersonatedAccountTxSubmitter extends EV5JsonRpcTxSubmitter { | |||||||
|
||||||||
constructor( | ||||||||
public readonly multiProvider: MultiProvider, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not reuse the multiprovider on the EV5JsonRpcTxSubmitter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No reason not to– updated to use parent's |
||||||||
public readonly chain: ChainName, | ||||||||
public readonly props: EV5ImpersonatedAccountTxSubmitterProps, | ||||||||
) { | ||||||||
super(multiProvider, chain); | ||||||||
super(multiProvider); | ||||||||
} | ||||||||
|
||||||||
public async submit( | ||||||||
...txs: PopulatedTransaction[] | ||||||||
): Promise<TransactionReceipt[]> { | ||||||||
const impersonatedAccount = await impersonateAccount(this.props.address); | ||||||||
this.multiProvider.setSigner(this.chain, impersonatedAccount); | ||||||||
super.multiProvider.setSigner(this.chain, impersonatedAccount); | ||||||||
this.multiProvider.setSharedSigner(impersonatedAccount); | ||||||||
super.multiProvider.setSharedSigner(impersonatedAccount); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we just reuse supers?
Suggested change
|
||||||||
return await super.submit(...txs); | ||||||||
Comment on lines
+35
to
36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. idk what the implication is of not doing this but should we There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting- I think we should do this in order to keep the submission atomic, and I don't think this should have any downstream effects since we are also impersonating during this call will address as a part of #3786 |
||||||||
} | ||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate changesets for the CLI and SDK, very nice thank you.