Skip to content

feat(bridge-withdrawer): bridge withdrawer startup#1160

Merged
itamarreif merged 13 commits intomainfrom
itamarreif/bridge-withdrawer-startup
Jun 8, 2024
Merged

feat(bridge-withdrawer): bridge withdrawer startup#1160
itamarreif merged 13 commits intomainfrom
itamarreif/bridge-withdrawer-startup

Conversation

@itamarreif
Copy link
Contributor

@itamarreif itamarreif commented Jun 6, 2024 •
edited
Loading

Summary

We need to validate config and balance before starting to process transactions.

  1. Added a startup() function to the watcher and submitter each, where set_ready() is called at the end of them. before, we were calling set_ready() before confirming the config against the node and establishing connection with geth.
  2. To keep the sequencer and geth logic separate, there's a one shot channel from the submitter to the watcher that is used for passing the valid fee asset, so the watcher waits for this from the submitter before starting its own startup logic
    a. watcher startup: connecting to geth, confirming contract addr, getting decimals from contract, making the batcher object
    b. submitter startup: confirming chain_id against sequencer node, making sure the fee asset is valid, checking that it meets a minimum balance (also passed from config)
  3. refactored some of the submitter unit tests/mock test things to be able to test the startup logic and added a sanity check test + for each of the startup requests failing

Background

Need to implement startup routine for bridge withdrawer.

Changes

  • submitter

    • confirm chain_id
    • confirm fee asset is valid
    • confirm sufficient balance of fee asset
      • add config var

  • watcher

    • get fee asset from submitter?
    • fix connection to geth to use exponential backoff
    • validate decimals against contract

  • fix set_ready() calls to be at the end of startup()

Testing

  • sanity check unit test that the startup works

Metrics

  • List out metrics added by PR, delete section if none.

Breaking Changelist

  • Bulleted list of breaking changes, any notes on migration. Delete section if none.

Related Issues

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

closes

@itamarreif itamarreif self-assigned this Jun 6, 2024
@itamarreif itamarreif marked this pull request as ready for review June 7, 2024 18:44
@itamarreif itamarreif requested review from a team as code owners June 7, 2024 18:44
noot
noot reviewed Jun 7, 2024
View reviewed changes
crates/astria-bridge-withdrawer/src/config.rs Outdated
// The fee asset denomination to use for the bridge account's transactions.
pub fee_asset_denomination: String,
// The minimum expected balance of the fee asset in the bridge account.
// TODO: This should be a u128, but the test fails to parse it as such.
Copy link
Contributor

@noot noot Jun 7, 2024

Choose a reason for hiding this comment

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

which test?

noot
noot reviewed Jun 7, 2024
View reviewed changes
crates/astria-bridge-withdrawer/src/withdrawer/ethereum/watcher.rs Outdated
.call()
.await
.wrap_err("failed to get asset withdrawal decimals")?;
let asset_withdrawal_divisor = 10u128.pow(base_chain_asset_precision);
Copy link
Contributor

@noot noot Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
let asset_withdrawal_divisor = 10u128.pow(base_chain_asset_precision);
let asset_withdrawal_divisor = 10u128.pow(18u32.checked_sub(base_chain_asset_precision).expect("base_chain_asset_precision must be <= 18, as the contract constructor enforces \
this"));

noot
noot reviewed Jun 7, 2024
View reviewed changes
noot
noot reviewed Jun 7, 2024
View reviewed changes
noot
noot reviewed Jun 7, 2024
View reviewed changes
noot
noot reviewed Jun 7, 2024
View reviewed changes
noot
noot reviewed Jun 7, 2024
View reviewed changes
noot
noot suggested changes Jun 7, 2024
View reviewed changes
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.

please fix asset_withdrawal_divisor calculation, otherwise nothing major!

itamarreif and others added 2 commits June 7, 2024 16:47
@itamarreif @noot
Update crates/astria-bridge-withdrawer/src/withdrawer/ethereum/watche…
5a0b9a6 
…r.rs

Co-authored-by: noot <36753753+noot@users.noreply.github.com>
@itamarreif
Merge branch 'main' of github.com:astriaorg/astria into itamarreif/br…
034d39f 
…idge-withdrawer-startup
@itamarreif itamarreif changed the title Itamarreif/bridge withdrawer startup feat(bridge-withdrawer): bridge withdrawer startup Jun 7, 2024
noot
noot approved these changes Jun 8, 2024
View reviewed changes
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.

lgtm!

@itamarreif itamarreif enabled auto-merge June 8, 2024 00:18
@SuperFluffy
Copy link
Contributor

SuperFluffy commented Jun 8, 2024

All these startup logics completely ignore shutdown signals.

joroshiba
joroshiba approved these changes Jun 8, 2024
View reviewed changes
Copy link
Member

@joroshiba joroshiba left a comment

Choose a reason for hiding this comment

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

infra approval, lean on janis & elizabeth for rust side.

@itamarreif itamarreif added this pull request to the merge queue Jun 8, 2024
Merged via the queue into main with commit d70fa98 Jun 8, 2024
@itamarreif itamarreif deleted the itamarreif/bridge-withdrawer-startup branch June 8, 2024 18:51
steezeburger added a commit that referenced this pull request Jun 10, 2024
@steezeburger
Merge branch 'main' into fix/hermes-chart
537c0a2 
* main:
  fix: ignore RUSTSEC-2021-0139 (#1171)
  chore(sequencer-relayer)!: remove functionality to restrict relaying blocks to only those proposed by a given validator (#1168)
  chore(metrics): update `metric_name` macro to handle a collection of names (#1163)
  fix(bridge-withdrawer): skip linting generated contract code (#1172)
  fix(core, sequencer): prefix removal source non-refund ics20 packet (#1162)
  chore(docs): add sequencer-relayer doc to specs (#1126)
  feat(bridge-withdrawer): sync logic (#1165)
  chore(withdrawer): replace contracts with `astria-bridge-contracts` submodule (#1164)
  feat(sequencer)!: implement bridge sudo and withdrawer addresses (#1142)
  feat(sequencer): implement refund to rollup logic upon ics20 transfer refund (#1161)
  feat(bridge-withdrawer): bridge withdrawer startup (#1160)
  feat(core, proto)!: add bech32m addresses (#1124)
  feat(withdrawer): bridged ERC20 token withdrawals (#1149)
  feat(sequencer-relayer)!: add chain IDs for sequencer and Celestia to config env vars (#1063)
  test(bridge-withdrawer): add submitter tests (#1133)
  chore: bump penumbra deps (#1159)
  feat(sequencer): implement `bridge/account_last_tx_hash` abci query (#1158)
  fix(withdrawer): use block subscription in batcher; send to destination_chain_address (#1157)
  fix(withdrawer): update AstriaWithdrawer to check that withdrawal value is sufficient (#1148)
  chore(ci): build bridge withdrawer images (#1156)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joroshiba joroshiba joroshiba approved these changes

@WafflesVonMaple WafflesVonMaple Awaiting requested review from WafflesVonMaple WafflesVonMaple is a code owner automatically assigned from astriaorg/infra

+1 more reviewer

@noot noot noot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@itamarreif itamarreif

Labels

bridging

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@itamarreif @SuperFluffy @joroshiba @noot