Create CLI Submitter #3730

nbayindirli · 2024-05-07T21:51:44Z

Description

A CLI-side submitter to interact with the SDK's tx submitter builder

Drive-by changes

None

Related issues

Fixes Implement CLI Submitter #3738

Backward compatibility

Yes

Testing

None

changeset-bot · 2024-05-07T21:51:47Z

🦋 Changeset detected

Latest commit: de28885

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages

Name	Type
@hyperlane-xyz/sdk	Minor
@hyperlane-xyz/cli	Minor
@hyperlane-xyz/helloworld	Minor
@hyperlane-xyz/infra	Minor
@hyperlane-xyz/core	Minor
@hyperlane-xyz/ccip-server	Minor
@hyperlane-xyz/utils	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2024-05-07T22:04:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.96%. Comparing base (344a716) to head (de28885).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3730   +/-   ##
=======================================
  Coverage   63.96%   63.96%           
=======================================
  Files         124      124           
  Lines        1543     1543           
  Branches      167      167           
=======================================
  Hits          987      987           
  Misses        530      530           
  Partials       26       26

Components	Coverage Δ
core	`84.61% <ø> (ø)`
hooks	`67.50% <ø> (ø)`
isms	`39.52% <ø> (ø)`
token	`65.41% <ø> (ø)`
middlewares	`77.05% <ø> (ø)`

jmrossy

Mostly looks good! Just a few small questions/comments to clear up.

I'd also appreciate it if we could exercise this in the ci-test.sh e2e test please. It's been useful for catching CLI regressions in the past.

jmrossy · 2024-05-09T15:07:09Z

.changeset/sixty-avocados-double.md

@@ -0,0 +1,5 @@
+---


Separate changesets for the CLI and SDK, very nice thank you.

jmrossy · 2024-05-09T15:08:58Z

typescript/cli/src/submit/submitTypes.ts

@@ -0,0 +1,33 @@
+import type {


Convention is to call this file just types and let it's location provide the context

Ah– I thought I refactored this back to types. Updated

jmrossy · 2024-05-09T15:10:29Z

typescript/cli/src/submit/submit.ts

+          'Must provide EV5ImpersonatedAccountTxSubmitterProps for impersonated account submitter.',
+        );
+
+      await verifyAnvil();


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?

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

i dont think so

jmrossy · 2024-05-09T15:11:50Z

typescript/cli/src/submit/submit.ts

+      if (!settings)
+        throw new Error(
+          'Must provide EV5ImpersonatedAccountTxSubmitterProps for impersonated account submitter.',
+        );


Check out the little assert function in our utils package.
assert(settings, 'EV5ImpersonatedAccountTxSubmitterProps required for impersonated account submitter')

For whatever reason it seems like we were using this convention more CLI-side, and assert SDK-side. In either case, now using the utils assert!

the one downside is its slightly less readable to the average TS dev but I like the brevity :)

jmrossy · 2024-05-09T15:12:44Z

typescript/sdk/src/providers/transactions/submitter/TxSubmitterTypes.ts

 export enum TxSubmitterType {
  JSON_RPC = 'JSON RPC',
  IMPERSONATED_ACCOUNT = 'Impersonated Account',
  GNOSIS_SAFE = 'Gnosis Safe',
 }
+
+export interface EV5GnosisSafeTxSubmitterProps {
+  safeAddress: Address;


Should this be just address for consistency?

I do not think so, since safeAddress is definitely only used to set the safeAddress for proposals

With that said, I updated the below field on EV5ImpersonatedAccountTxSubmitterProps to be userAddress for symmetry (also more accurate)

yorhodes

great start
I think we can cut down further on the number of new config interfaces here

yorhodes · 2024-05-09T20:36:49Z

typescript/cli/src/submit/submit.ts

+      if (!settings)
+        throw new Error(
+          'Must provide EV5ImpersonatedAccountTxSubmitterProps for impersonated account submitter.',
+        );


the one downside is its slightly less readable to the average TS dev but I like the brevity :)

yorhodes · 2024-05-09T20:39:39Z

typescript/cli/src/submit/submit.ts

+          'Must provide EV5ImpersonatedAccountTxSubmitterProps for impersonated account submitter.',
+        );
+
+      await verifyAnvil();


i dont think so

yorhodes · 2024-05-09T20:40:33Z

typescript/cli/src/submit/submit.ts

+      if (!settings)
+        throw new Error(
+          'Must provide EV5GnosisSafeTxSubmitterProps for Gnosis safe submitter.',
+        );


could we have the required settings somehow be derived from the discriminated union?
ie the JSON_RPC submitter has an empty/undefined settings object etc

we could also use zod schemas here to validate the input

yorhodes · 2024-05-09T20:42:13Z

typescript/cli/src/submit/submit.ts

+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>(


I think these do not need to be async

this doesn't look resolved?

this doesn't look resolved?

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

the async transform doesnt happen in this function iiuc

yorhodes · 2024-05-09T20:49:01Z

typescript/cli/src/submit/submitTypes.ts

+export interface SubmitterMetadata {
+  type: TxSubmitterType;
+  settings?: SubmitterSettings;
+}


can the settings type be derived from the TxSubmitterType?

Suggested change

export interface SubmitterMetadata {

type: TxSubmitterType;

settings?: SubmitterSettings;

}

export interface SubmitterMetadata<T extends TxSubmitterType> {

type: T;

settings?: SubmitterSettings<T>;

}

something like this

and at that point submitter metadata seems superfluous with the submitter settings interface itself

yorhodes · 2024-05-09T20:56:01Z

...cript/sdk/src/providers/transactions/submitter/ethersV5/EV5ImpersonatedAccountTxSubmitter.ts

+    this.multiProvider.setSharedSigner(impersonatedAccount);
+    super.multiProvider.setSharedSigner(impersonatedAccount);


can we just reuse supers?

Suggested change

this.multiProvider.setSharedSigner(impersonatedAccount);

super.multiProvider.setSharedSigner(impersonatedAccount);

super.multiProvider.setSharedSigner(impersonatedAccount);

yorhodes · 2024-05-09T20:59:09Z

typescript/sdk/src/providers/transactions/transformer/TxTransformerTypes.ts

+export interface EV5InterchainAccountTxTransformerProps {
+  chain: ChainName;
+  interchainAccount: InterchainAccount;
+  accountConfig: AccountConfig;
+  hookMetadata?: string;
+}


would it be possible to just say Parameters<typeof InterchainAccount['callRemote'] or something?

this is definitely possible, but since getCallRemote's args aren't destructed, we'd have to index into the parameter array (instead of using type names) or refactor to ICA class. Additionally, we set destination & innerCalls based on the txs (both params of getCallRemote), which seems like an antipattern if we have to write back to props (or avoid using two of its fields)

yorhodes · 2024-05-09T21:00:13Z

...ipt/sdk/src/providers/transactions/transformer/ethersV5/EV5InterchainAccountTxTransformer.ts

    const innerCalls: CallData[] = txs.map(
-      ({ to, data, value }: PopulatedTransaction) => {
+      ({ to, data, value, chainId }: PopulatedTransaction) => {


yorhodes · 2024-05-09T21:00:24Z

...ipt/sdk/src/providers/transactions/transformer/ethersV5/EV5InterchainAccountTxTransformer.ts

        assert(to, 'Invalid PopulatedTransaction: Missing to field');
        assert(data, 'Invalid PopulatedTransaction: Missing data field');
+        assert(chainId, 'Invalid PopulatedTransaction: Missing chainId field');
+        const txChain = this.multiProvider.getChainName(chainId);
+        assert(
+          txChain === this.props.chain,
+          `Invalid PopulatedTransaction: Cannot submit ${txChain} tx to ${this.props.chain} submitter.`,
+        );


I wonder if we can leverage zod schemas here too

yorhodes · 2024-05-09T21:00:47Z

...ipt/sdk/src/providers/transactions/transformer/ethersV5/EV5InterchainAccountTxTransformer.ts

-        this.chain,
-        this.multiProvider.getChainName(this.chain), //chainIdToMetadata[destinationChainId].name,
+        this.props.chain,
+        this.multiProvider.getChainName(txs[0].chainId!),


I suppose we should assert that the chain Id is consistent, or actually produce separate callRemote bundles for different chain Ids

probably the latter

yep love this idea– added a network -> tx bundle mapping to broadcast

jmrossy

✅
Recommend waiting for Yorke's approval too :)

…g (zod-support next)

yorhodes · 2024-05-14T21:16:59Z

...cript/sdk/src/providers/transactions/submitter/ethersV5/EV5ImpersonatedAccountTxSubmitter.ts

+    super.multiProvider.setSharedSigner(impersonatedAccount);
    return await super.submit(...txs);


idk what the implication is of not doing this but should we
stopImpersonating after submitting?

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

yorhodes · 2024-05-14T21:18:12Z

typescript/sdk/src/providers/transactions/submitter/ethersV5/EV5TxSubmitterTypes.ts

+export interface EV5GnosisSafeTxSubmitterProps {
+  chain: ChainName;
+  safeAddress: Address;
+}
+
+export interface EV5ImpersonatedAccountTxSubmitterProps {
+  chain: ChainName;
+  userAddress: Address;
+}


is chain needed for every submitter?
maybe we can omit that from each

not actually – EV5ImpersonatedAccountTxSubmitterProps does not actually need a specific network tied to it, so will remove chain here. Gnosis does need it in order to verify the submitted transactions can be used with that submitter

will address as a part of #3786

nbayindirli requested review from jmrossy, yorhodes and aroralanuk as code owners May 7, 2024 21:51

nbayindirli marked this pull request as draft May 7, 2024 21:52

add CLI submitter

038966b

nbayindirli force-pushed the noah/cli-submitter branch from 70328a1 to 038966b Compare May 8, 2024 21:23

nbayindirli changed the title ~~[WIP] CLI-Submitter Integration~~ Create CLI Submitter May 8, 2024

nbayindirli marked this pull request as ready for review May 8, 2024 21:23

nbayindirli mentioned this pull request May 8, 2024

Implement CLI Submitter #3738

Closed

nbayindirli added 2 commits May 8, 2024 17:53

export submitter/transformer props from SDK for use in CLI-submitter

4849c80

add changesets

6865bd1

nbayindirli requested review from Skunkchain, nambrot and avious00 as code owners May 8, 2024 21:55

jmrossy reviewed May 9, 2024

View reviewed changes

nbayindirli added 3 commits May 9, 2024 15:25

decouple 'chain' from submitters and transformers

7d25842

Merge branch 'main' into noah/cli-submitter

8e4ff93

address Rossy's comments

3d204af

nbayindirli requested a review from jmrossy May 9, 2024 20:51

yorhodes reviewed May 9, 2024

View reviewed changes

jmrossy approved these changes May 10, 2024

View reviewed changes

nbayindirli added 7 commits May 10, 2024 14:15

use descriminated union to improve branching and remove error handlin…

c424ca3

…g (zod-support next)

Merge branch 'main' into noah/cli-submitter

5fdaf9a

move EV5 submitter/transformer types into ethersV5 dir

2806734

reuse parent class mp

5aee461

add ICA innerCall batching based on network

1f40e36

Merge branch 'main' into noah/cli-submitter

9bb15ce

Merge branch 'main' into noah/cli-submitter

8296880

Merge branch 'main' into noah/cli-submitter

de28885

yorhodes approved these changes May 14, 2024

View reviewed changes

nbayindirli merged commit eba3936 into main May 15, 2024
34 checks passed

nbayindirli deleted the noah/cli-submitter branch May 15, 2024 14:14

		this.multiProvider.setSharedSigner(impersonatedAccount);
		super.multiProvider.setSharedSigner(impersonatedAccount);

		super.multiProvider.setSharedSigner(impersonatedAccount);
		return await super.submit(...txs);

Create CLI Submitter #3730

Create CLI Submitter #3730

Conversation

nbayindirli commented May 7, 2024 • edited Loading

Description

Drive-by changes

Related issues

Backward compatibility

Testing

changeset-bot bot commented May 7, 2024 • edited Loading

🦋 Changeset detected

codecov bot commented May 7, 2024 • edited Loading

Codecov Report

jmrossy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorhodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmrossy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbayindirli commented May 7, 2024 •

edited

Loading

changeset-bot bot commented May 7, 2024 •

edited

Loading

codecov bot commented May 7, 2024 •

edited

Loading

jmrossy left a comment •

edited

Loading