Skip to content

refactor(bridge-withdrawer)!: refactor startup to a separate subtask and remove balance check from startup#1190

Merged
itamarreif merged 46 commits intomainfrom
itamarreif/bridge-withdrawer-startup-fixes
Jul 9, 2024
Merged

refactor(bridge-withdrawer)!: refactor startup to a separate subtask and remove balance check from startup#1190
itamarreif merged 46 commits intomainfrom
itamarreif/bridge-withdrawer-startup-fixes

Conversation

@itamarreif
Copy link
Contributor

@itamarreif itamarreif commented Jun 17, 2024 •
edited
Loading

Summary

This refactors some of the startup logic into a separate startup subtask. It also removes the minimum expected balance check from the startup logic, as that is not necessary.

Background

The original startup logic had the Submitter running parts of the startup routine that weren't relevant to it. this moves that logic out to the Startup task.

The balance check is removed because if a sequencer transaction with insufficient balance is submitted it will just fail execution and the service will restart. Requiring the check in the startup logic results in clunky operation of the service, since launching it before funding it will cause it to crash.

Changes

  • Introduce Startup object and task
  • Watcher and Submitter now wait for the Startup to update the global state object with startup info before running their startup logic, whereas before we just had the Watcher wait on the Submitter to send a message on the oneshot channel.
  • Cleaned up some of the tryhard configs with helper funcs to be less confusing

Breaking Changes

  • Remove the balance check from startup and the associated configs. issue linked below

Testing

  • Updated some of the Submitter-specific tests to reflect changes to the startup logic.

closes #1229

@itamarreif itamarreif changed the title Itamarreif/bridge withdrawer startup fixes refactor(bridge-withdrawer): refactor startup to a separate subtask Jun 17, 2024
@itamarreif itamarreif marked this pull request as ready for review June 18, 2024 21:20
@itamarreif itamarreif requested a review from a team as a code owner June 18, 2024 21:20
@itamarreif itamarreif requested a review from Fraser999 June 18, 2024 21:20
@itamarreif itamarreif self-assigned this Jun 18, 2024
noot
noot reviewed Jun 20, 2024
View reviewed changes
noot
noot reviewed Jun 20, 2024
View reviewed changes
noot
noot reviewed Jun 20, 2024
View reviewed changes
noot
noot reviewed Jun 20, 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.

overall looks good, only some minor comments!

@itamarreif itamarreif requested a review from noot June 24, 2024 22:33
@itamarreif itamarreif changed the title refactor(bridge-withdrawer): refactor startup to a separate subtask and remove balance check from startup refactor(bridge-withdrawer)!: refactor startup to a separate subtask and remove balance check from startup Jul 8, 2024
@github-actions github-actions bot added the cd label Jul 8, 2024
@itamarreif itamarreif requested review from a team and WafflesVonMaple July 8, 2024 12:59
@itamarreif itamarreif added this pull request to the merge queue Jul 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 8, 2024
@itamarreif itamarreif added this pull request to the merge queue Jul 9, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 9, 2024
@itamarreif
refactor(bridge-withdrawer)!: refactor startup to a separate subtask …
78ea2aa 
…and remove balance check from startup (#1190)

## Summary
This refactors some of the startup logic into a separate startup
subtask. It also removes the minimum expected balance check from the
startup logic, as that is not necessary.

## Background
The original startup logic had the `Submitter` running parts of the
startup routine that weren't relevant to it. this moves that logic out
to the `Startup` task.

The balance check is removed because if a sequencer transaction with
insufficient balance is submitted it will just fail execution and the
service will restart. Requiring the check in the startup logic results
in clunky operation of the service, since launching it before funding it
will cause it to crash.

## Changes
- Introduce `Startup` object and task
- `Watcher` and `Submitter` now wait for the `Startup` to update the
global state object with startup info before running their startup
logic, whereas before we just had the `Watcher` wait on the `Submitter`
to send a message on the `oneshot` channel.
- Cleaned up some of the tryhard configs with helper funcs to be less
confusing

## Breaking Changes
- Remove the balance check from startup and the associated configs.
issue linked below

## Testing
- Updated some of the `Submitter`-specific tests to reflect changes to
the startup logic.

closes #1229
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 9, 2024
@joroshiba joroshiba added the docker-build used to trigger docker builds on PRs label Jul 9, 2024
@itamarreif itamarreif added this pull request to the merge queue Jul 9, 2024
@itamarreif itamarreif removed this pull request from the merge queue due to a manual request Jul 9, 2024
@itamarreif itamarreif added this pull request to the merge queue Jul 9, 2024
Merged via the queue into main with commit df76869 Jul 9, 2024
@itamarreif itamarreif deleted the itamarreif/bridge-withdrawer-startup-fixes branch July 9, 2024 12:08
steezeburger added a commit that referenced this pull request Jul 15, 2024
@steezeburger
Merge branch 'main' into fix/hermes-chart
5a3c8e7 
* main:
  feat(cli): add cmd to collect withdrawal events and submit as actions (#1261)
  fix(core, bridge, sequencer)!: dismabiguate return addresses (#1266)
  fix(withdrawer): support withdrawer address that differs from bridge address   (#1262)
  (core, sequencer)!: generate serde traits impls for all protocol protobufs (#1260)
  fix(charts): add resources for sequencer/cometbft (#1254)
  chore(sequencer)!: add metrics (#1248)
  fix(sequencer-utils): fixes issue in `parse_blob` tests (#1243)
  feat(core, proto)!: make bridge unlock memo string (#1244)
  fix(conductor): don't panic during panic (#1252)
  feat(core)!: lowerCamelCase for protobuf json mapping (#1250)
  refactor(bridge-withdrawer)!: refactor startup to a separate subtask and remove balance check from startup (#1190)
  fix: rollup archive node configurations (#1249)
  refactor(core, bridge-withdrawer)!: move bridge-unlock memo to core (#1245)
  fix(sequencer)!: store native asset ibc->trace mapping in init_chain (#1242)
steezeburger added a commit that referenced this pull request Jul 19, 2024
@steezeburger
Merge branch 'main' into joroshiba/breakup-rollup
ec89454 
* main: (24 commits)
  chore: update `bytes` and `ics23` crates (#1279)
  fix(sequencer): improve and fix instrumentation (#1255)
  feature(charts): hermes chart fixes, bech32 updates, ibc bridge test (#1130)
  chore(cli): remove unused rollup cli code (#1275)
  chore(test): use a temporary file to not pollute the workspace (#1269)
  chore(sequencer): add mempool benchmarks (#1238)
  fix(bridge-withdrawer)!: fix nonce handling (#1215)
  feat(cli, bridge-withdrawer)!: share code between cli and service (#1270)
  feat(cli): add cmd to collect withdrawal events and submit as actions (#1261)
  fix(core, bridge, sequencer)!: dismabiguate return addresses (#1266)
  fix(withdrawer): support withdrawer address that differs from bridge address   (#1262)
  (core, sequencer)!: generate serde traits impls for all protocol protobufs (#1260)
  fix(charts): add resources for sequencer/cometbft (#1254)
  chore(sequencer)!: add metrics (#1248)
  fix(sequencer-utils): fixes issue in `parse_blob` tests (#1243)
  feat(core, proto)!: make bridge unlock memo string (#1244)
  fix(conductor): don't panic during panic (#1252)
  feat(core)!: lowerCamelCase for protobuf json mapping (#1250)
  refactor(bridge-withdrawer)!: refactor startup to a separate subtask and remove balance check from startup (#1190)
  fix: rollup archive node configurations (#1249)
  ...
bharath-123 pushed a commit that referenced this pull request Jul 25, 2024
@itamarreif @bharath-123
refactor(bridge-withdrawer)!: refactor startup to a separate subtask …
1af6474 
…and remove balance check from startup (#1190)

## Summary
This refactors some of the startup logic into a separate startup
subtask. It also removes the minimum expected balance check from the
startup logic, as that is not necessary.

## Background
The original startup logic had the `Submitter` running parts of the
startup routine that weren't relevant to it. this moves that logic out
to the `Startup` task.

The balance check is removed because if a sequencer transaction with
insufficient balance is submitted it will just fail execution and the
service will restart. Requiring the check in the startup logic results
in clunky operation of the service, since launching it before funding it
will cause it to crash.

## Changes
- Introduce `Startup` object and task
- `Watcher` and `Submitter` now wait for the `Startup` to update the
global state object with startup info before running their startup
logic, whereas before we just had the `Watcher` wait on the `Submitter`
to send a message on the `oneshot` channel.
- Cleaned up some of the tryhard configs with helper funcs to be less
confusing

## Breaking Changes
- Remove the balance check from startup and the associated configs.
issue linked below

## Testing
- Updated some of the `Submitter`-specific tests to reflect changes to
the startup logic.

closes #1229
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fraser999 Fraser999 Awaiting requested review from Fraser999 Fraser999 is a code owner automatically assigned from astriaorg/rust-reviewers

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

3 more reviewers

@SuperFluffy SuperFluffy SuperFluffy approved these changes

@noot noot noot approved these changes

@quasystaty1 quasystaty1 quasystaty1 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@itamarreif itamarreif

Labels

bridging cd docker-build used to trigger docker builds on PRs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove balance check from startup in bridge-withdrawer

5 participants

@itamarreif @SuperFluffy @noot @quasystaty1 @joroshiba