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

STR-1180: remove legacy bridge client #737

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

delbonis
Copy link
Contributor

Description

Focus areas:

  • removed bridge-client binary
  • removed bridge related crates
  • remove references to old bridge infra from fntest envs

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor
  • New or updated tests
  • Dependency Update

Notes to Reviewers

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added (where necessary) tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

@delbonis delbonis requested review from a team as code owners March 13, 2025 20:31
Copy link
Contributor

@Rajil1213 Rajil1213 left a comment

Choose a reason for hiding this comment

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

Does this PR cover both the purging of the bridge crates and the changes to the deposit table?

The actual removal of all the bridge crates looks good. The tests directory can be removed too as that only contains integration tests for the bridge. And the docker files can be purged as well (including the bridge-client section in the compose file).

@delbonis delbonis requested a review from a team as a code owner March 13, 2025 22:43
@delbonis
Copy link
Contributor Author

@Rajil1213 If you mean the deposit tag then I did that in the previous PR that I merged already. If not then I'm not sure?

@delbonis delbonis requested a review from a team as a code owner March 13, 2025 22:46
@delbonis delbonis force-pushed the STR-1180-remove-legacy-bridge-client branch from 475fc3c to ff7fa35 Compare March 13, 2025 22:50
@delbonis
Copy link
Contributor Author

Does it make sense to keep the "deposit" subcommand in strata-cli?

@Rajil1213
Copy link
Contributor

If you mean the deposit tag then I did that in the previous PR that I merged already. If not then I'm not sure?

yeah, maybe the main branch had not been updated when I viewed it.

Does it make sense to keep the "deposit" subcommand in strata-cli?

Yes, afaik we are still gonna use it for deposits.

@Rajil1213
Copy link
Contributor

The integration.yml workflow in .github/workflows can also be removed btw.

Copy link

codecov bot commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.

Project coverage is 52.91%. Comparing base (aeda4e4) to head (145919b).

Files with missing lines Patch % Lines
crates/consensus-logic/src/fork_choice_manager.rs 0.00% 8 Missing ⚠️
bin/strata-client/src/rpc_server.rs 0.00% 7 Missing ⚠️
bin/strata-client/src/main.rs 0.00% 1 Missing ⚠️
@@            Coverage Diff             @@
##             main     #737      +/-   ##
==========================================
- Coverage   54.84%   52.91%   -1.93%     
==========================================
  Files         322      298      -24     
  Lines       36207    32564    -3643     
==========================================
- Hits        19858    17232    -2626     
+ Misses      16349    15332    -1017     
Files with missing lines Coverage Δ
bin/strata-cli/src/cmd/mod.rs 0.00% <ø> (ø)
bin/strata-cli/src/main.rs 0.00% <ø> (ø)
bin/strata-client/src/args.rs 86.33% <ø> (-0.48%) ⬇️
crates/config/src/config.rs 90.47% <ø> (-0.44%) ⬇️
crates/db/src/traits.rs 0.00% <ø> (ø)
crates/l1tx/src/deposit/deposit_request.rs 98.36% <ø> (ø)
crates/l1tx/src/deposit/deposit_tx.rs 94.23% <ø> (ø)
crates/rocksdb-store/src/lib.rs 4.76% <ø> (ø)
crates/rpc/types/src/types.rs 0.00% <ø> (ø)
crates/test-utils/src/bridge.rs 91.60% <ø> (-3.37%) ⬇️
... and 3 more

... and 16 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented Mar 13, 2025

Commit: ba597b8

SP1 Execution Results

program cycles success
Bitcoin Blockspace 71,012
EVM EE STF 301,434
CL STF 166,633
Checkpoint 4,252

@delbonis
Copy link
Contributor Author

Current status is that some remaining tests need to be disabled but it seems most pieces are working as expected. Also need to resolve clippy warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants