Skip to content

feat(bridge-withdrawer): Bridge withdrawer transaction submission#1087

Merged
noot merged 25 commits intoitamarreif/bridge-scaffoldingfrom
itamarreif/bridge-executor
May 24, 2024
Merged

feat(bridge-withdrawer): Bridge withdrawer transaction submission#1087
noot merged 25 commits intoitamarreif/bridge-scaffoldingfrom
itamarreif/bridge-executor

Conversation

@itamarreif
Copy link
Contributor

@itamarreif itamarreif commented May 20, 2024

Summary

This adds the Submitter and merges in the Watcher related logic into a single WithdrawService.

Submitter receives batches of rollup transactions that have been converted into sequencer Actions from the Batcher and does the following:

  1. fetch the current nonce and create the UnsignedTransaction
  2. sign the transaction
  3. broadcast_tx_commit

If submission to the sequencer fails for any reason (either in CheckTx or DeliverTx), the Submitter will stop, instead relying on the service's recovery process to reconstruct the batch from rollup transaction and resubmit.

Background

Withdrawal batches need to be submitted to the sequencer.

Changes

  • Add Submitter
  • Add logic for converting the events reaped from rollup to Actions
  • a lot of random cleanup

Testing

How are these changes tested?

Metrics

  • Some metrics we use in composer already since i copied over a good amount of submission logic:
    • NONCE_FETCH_COUNT
    • NONCE_FETCH_FAILURE_COUNT
    • NONCE_FETCH_LATENCY
    • CURRENT_NONCE
    • SEQUENCER_SUBMISSION_FAILURE_COUNT
    • SEQUENCER_SUBMISSION_LATENCY

Related Issues

Link any issues that are related, prefer full github links.

closes

@itamarreif itamarreif changed the title feat(bridge-withdraw-relay): Bridge withdrawer transaction submission feat(bridge-withdrawer): Bridge withdrawer transaction submission May 22, 2024
@itamarreif itamarreif marked this pull request as ready for review May 22, 2024 23:20
@itamarreif itamarreif requested a review from a team as a code owner May 22, 2024 23:20
@itamarreif itamarreif requested a review from SuperFluffy May 22, 2024 23:20
@itamarreif itamarreif requested a review from a team as a code owner May 22, 2024 23:30
@github-actions github-actions bot added conductor pertaining to the astria-conductor crate sequencer-relayer pertaining to the astria-sequencer-relayer crate composer pertaining to composer labels May 23, 2024
Comment on lines 215 to 218
Copy link
Contributor

Choose a reason for hiding this comment

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

the event.sender is an evm address - we need to parse the event.memo to get the destination sequencer address (or check if it's an ics20withdrawal)

also thinking amount needs a "precision" as well, as going to the evm for example we multiply by some precision so here we'd need to divide by that precision

memo should also be updated to store the evm block number and tx hash

Copy link
Contributor

@noot noot May 23, 2024

Choose a reason for hiding this comment

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

i propose the evm tx memo looks like:

{
    "astria": {
         "recipient_address": "astriaaddress",
    },
    "forward": {
         "receiver": "destinationchainbech32address",
         "port": "transfer",
         "channel": "channel-123"
    }
} 

where forward is set only if this is an ics20 withdrawal, and astria can be unset. if it's a withdrawal to a sequencer account, then astria is set and forward is unset

Copy link
Contributor

@noot noot May 23, 2024

Choose a reason for hiding this comment

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

i also propose the memo for BridgeUnlockAction looks like

{
     "rollup_block_number": 99,
     "rollup_tx_hash": "tx_hash"
}

tx_hash comes from event_with_metadata.

Copy link
Contributor

@noot noot May 23, 2024

Choose a reason for hiding this comment

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

no more evm tx memo for withdrawals to sequencer, see #1101

for ibc withdrawals, channel/port are hardcoded. the ibc packet memo should contain whatever is in the Ics20Withdrawal event memo plus additional data:

{
     "rollup_block_number": 99,
     "rollup_tx_hash": "tx_hash",
     "rollup_sender_address": "0x..."
}

Copy link
Contributor

@noot noot 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, my main concerns before merging:

  • fetching chain id from sequencer instead of config
  • fetching fee asset id from sequencer or setting in config instead of hardcoding
  • conversion of event into action needs to be updated

@noot noot merged commit 75a1b29 into itamarreif/bridge-scaffolding May 24, 2024
@noot noot deleted the itamarreif/bridge-executor branch May 24, 2024 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bridging composer pertaining to composer conductor pertaining to the astria-conductor crate sequencer-relayer pertaining to the astria-sequencer-relayer crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments