Skip to content
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

feat: Modular Tx Submission – Create Transformer + Submitter + Builder Abstraction #3627

Merged
merged 27 commits into from
May 1, 2024

Conversation

nbayindirli
Copy link
Contributor

@nbayindirli nbayindirli commented Apr 19, 2024

Description

  • Adds modular transaction submission support for SDK clients, e.g. CLI.

To-do:

  • Failing CI build due to linting Gnosis Safe package import
  • Export to sdk/src/index.ts

Note:

Transformers

  • Input: PopulatedTx[]
  • Output: HTX[] (where HTX extends HyperlaneTx)
  • Purpose: Given a set of populated transactions, transform those transactions into a set a HyperlaneTxs (for the corresponding submitter), e.g.
...
const somePopulatedTxs = ...
const transformer = new InterchainAccountTxTransformer(mp,o,d,p);
const populatedTxs = transformer.transformTxs(somePopulatedTxs);

Submitters

  • Input: HTX[] (where HTX extends HyperlaneTx)
  • Output: TxReceipt[] | ResponseData[]
  • Purpose: Given a set of Hyperlane transactions, execute those transactions for the specified submitter (submitter of type HTX should enforce the transactions being passed are of type HTX), e.g.
...
const submitter = new GnosisSafeTxSubmitter(mp,c,p);
const txReceipts =  submitter.submitTxs(populatedTxs);
---
Client-side example: for each gnosisTxReceipt display transactionHash

Builder (Utilizes both Submitters & Transformer)

  • Input: (TxTransformer<HTX> | TxTransformerType & Chain) & (TxSubmitter<HTX,HTR> | TxSubmitterType & Chain) & HTX[] (where HTX extends HyperlaneTx)
  • Output: TxReceipt[] | *response data*
  • Purpose: Given a submitter, an optional transformer, and a set of PopulatedTransactions, transform and submit all transactions, e.g.
...
const eV5builder = new TxSubmitterBuilder<EV5Transaction, EV5TransactionReceipt>();
let txReceipts = eV5builder.for(
  new GnosisSafeTxSubmitter(chainA)
).transform(
  InterchainAccountTxTransformer(chainB)
).submit(
  txs
);
txReceipts = eV5builder.for(
  new ImpersonatedAccountTxSubmitter(chainA)
).submit(txs);
txReceipts = eV5builder.for(
  new JsonRpcTxSubmitter(chainC)
).submit(txs);
---
Client-side example: for each txReceipt display transactionHash | response data

Drive-by changes

  • None

Related issues

Backward compatibility

  • Yes

Testing

  • Testing through CLI unit testing

Copy link

changeset-bot bot commented Apr 19, 2024

🦋 Changeset detected

Latest commit: a9ecb22

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

Copy link

codecov bot commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.28%. Comparing base (cf727db) to head (a9ecb22).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3627   +/-   ##
=======================================
  Coverage   74.28%   74.28%           
=======================================
  Files         108      108           
  Lines        1217     1217           
  Branches      132      132           
=======================================
  Hits          904      904           
  Misses        297      297           
  Partials       16       16           
Components Coverage Δ
core 84.61% <ø> (ø)
hooks 69.94% <ø> (ø)
isms 73.17% <ø> (ø)
token 63.77% <ø> (ø)
middlewares 77.48% <ø> (ø)

Copy link
Member

@yorhodes yorhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by the Tx type per submitter
ideally any TxSubmitter can take output from any Module.update(config)

@nbayindirli nbayindirli changed the title Modular Tx Submission: Create Transformer + Submitter + Builder Abstraction feat: Modular Tx Submission: Create Transformer + Submitter + Builder Abstraction Apr 23, 2024
@nbayindirli nbayindirli changed the title feat: Modular Tx Submission: Create Transformer + Submitter + Builder Abstraction feat: Modular Tx Submission – Create Transformer + Submitter + Builder Abstraction Apr 23, 2024
@nbayindirli nbayindirli marked this pull request as ready for review April 25, 2024 21:28
@nbayindirli nbayindirli requested a review from yorhodes April 25, 2024 21:30
Copy link
Member

@yorhodes yorhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

submitter and transformer implementations look good
I still think we need to streamline the builder interface a bit to be extensible to future implementations

@nbayindirli nbayindirli requested a review from yorhodes April 29, 2024 20:32
@nbayindirli
Copy link
Contributor Author

nbayindirli commented Apr 29, 2024

submitter and transformer implementations look good I still think we need to streamline the builder interface a bit to be extensible to future implementations

Agreed– with the newest update you'll find:

  • Comment resolution
  • Functionality to allow multiple transforms to occur for a single submit
  • Utilization of TypedTransaction & TypedTransactionReceipt, enabling the builder to be used cross-protocol (EV5, CW, Sealevel, etc.)
  • for is now used instead of add when specifying a submitter for the builder

Copy link
Contributor

@paulbalaji paulbalaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggested use of ProtocolTypedTransaction which could be useful across the submitter/builder classes

note: a drawback I can see of this is that if we decide to implement new submitter classes for different providers (e.g. viem/ethersv6), it would require tweaking some of the type maps - however I think this would be required to integrate these providers at the crud module level anyway

Copy link
Member

@yorhodes yorhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly looks good, please replace the stack-typescript implemenetation

@nbayindirli nbayindirli requested review from paulbalaji and yorhodes May 1, 2024 17:52
@nbayindirli
Copy link
Contributor Author

Note that this no longer builds due to registry work– merging this PR is therefore now blocked by migrating registry.ts to the SDK: #3689

@nbayindirli nbayindirli merged commit d37cbab into main May 1, 2024
34 of 35 checks passed
@nbayindirli nbayindirli deleted the noah/txs branch May 1, 2024 20:24
ltyu pushed a commit that referenced this pull request May 3, 2024
…r Abstraction (#3627)

## Description

- Adds modular transaction submission support for SDK clients, e.g. CLI.

To-do:
- [ ] Failing CI build due to linting Gnosis Safe package import
- [ ] Export to `sdk/src/index.ts`

Note: 
- Built to eventually expand to [Sealevel/CW
support](https://github.com/hyperlane-xyz/hyperlane-monorepo/blob/dc628476158672d08c766936ef4e2b150f86f566/typescript/sdk/src/providers/ProviderType.ts#L51-L85)

### Transformers
  - Input: `PopulatedTx[]`
  - Output: `HTX[]` (where `HTX extends HyperlaneTx`)
- Purpose: Given a set of populated transactions, transform those
transactions into a set a HyperlaneTxs (for the corresponding
submitter), e.g.
```
...
const somePopulatedTxs = ...
const transformer = new InterchainAccountTxTransformer(mp,o,d,p);
const populatedTxs = transformer.transformTxs(somePopulatedTxs);
```

### Submitters
  - Input: `HTX[]` (where `HTX extends HyperlaneTx`)
  - Output: `TxReceipt[] | ResponseData[]`
- Purpose: Given a set of Hyperlane transactions, execute those
transactions for the specified submitter (submitter of type HTX should
enforce the transactions being passed are of type HTX), e.g.
```
...
const submitter = new GnosisSafeTxSubmitter(mp,c,p);
const txReceipts =  submitter.submitTxs(populatedTxs);
---
Client-side example: for each gnosisTxReceipt display transactionHash
```

### Builder (Utilizes both Submitters & Transformer)
- Input: `(TxTransformer<HTX> | TxTransformerType & Chain) &
(TxSubmitter<HTX,HTR> | TxSubmitterType & Chain) & HTX[]` (where `HTX
extends HyperlaneTx`)
  - Output: `TxReceipt[] | *response data*`
- Purpose: Given a submitter, an optional transformer, and a set of
PopulatedTransactions, transform and submit all transactions, e.g.
```
...
const eV5builder = new TxSubmitterBuilder<EV5Transaction, EV5TransactionReceipt>();
let txReceipts = eV5builder.for(
  new GnosisSafeTxSubmitter(chainA)
).transform(
  InterchainAccountTxTransformer(chainB)
).submit(
  txs
);
txReceipts = eV5builder.for(
  new ImpersonatedAccountTxSubmitter(chainA)
).submit(txs);
txReceipts = eV5builder.for(
  new JsonRpcTxSubmitter(chainC)
).submit(txs);
---
Client-side example: for each txReceipt display transactionHash | response data
```

### Drive-by changes

* None

### Related issues

- Fixes #3547

### Backward compatibility

- Yes

### Testing

- Testing through CLI unit testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Epic][TxS] Modularize SDK TX Submission (w/ Transformers + Builder)
3 participants