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 754: Support Addresses and OP_RETURNs as withdrawal destinations #600

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

storopoli
Copy link
Member

@storopoli storopoli commented Jan 8, 2025

Description

Right now the bridge supports withdraws only to P2TR. The user passes an X-only PK to the bridge withdrawal precompile address.

However we need to support any arbitrary address, along with OP_RETURNs as valid withdrawal destinations.

TODO

  • functional tests for different BOSD.
  • fix EVM issues
  • pin bitcoin-bosd to a version in crates.io.

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 tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

STR-754

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

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

Project coverage is 56.82%. Comparing base (ce29ffc) to head (a9e7fce).

Files with missing lines Patch % Lines
crates/primitives/src/l1.rs 0.00% 8 Missing ⚠️
@@            Coverage Diff             @@
##             main     #600      +/-   ##
==========================================
- Coverage   56.85%   56.82%   -0.04%     
==========================================
  Files         308      308              
  Lines       32372    32379       +7     
==========================================
- Hits        18405    18398       -7     
- Misses      13967    13981      +14     
Files with missing lines Coverage Δ
crates/primitives/src/l1.rs 77.76% <0.00%> (-0.66%) ⬇️

... and 3 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Commit: 44cccc7

SP1 Performance Test Results

program cycles success
BTC_BLOCKSPACE 7,239,526
EL_BLOCK 102,449
CL_BLOCK 55,484
L1_BATCH 12,460,156
L2_BATCH 5,448
CHECKPOINT 15,322

@storopoli storopoli force-pushed the STR-754 branch 2 times, most recently from a9e7fce to 5471b8f Compare January 13, 2025 16:15
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Tweaks to representation.

crates/primitives/src/errors.rs Outdated Show resolved Hide resolved
Comment on lines 59 to 60
// FIXME: How to make this work with a BOSD Descriptor.
destination,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still probably pass it around as plain bytes instead of the encoded representation. Once we've validated it we don't have to introspect it again until the operator gets it. (Although we'd probably validate it once more when we have a cleaner break between the EVM EE and the Strata chain, still keeping it as bytes though.)

Comment on lines 26 to 27
// FIXME: How do I use a BOSD Descriptor here?
bytes32 destination,
Copy link
Contributor

Choose a reason for hiding this comment

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

bytes, unfortunately

This kinda still is messy, we shouldn't be using logs for this imo.

Comment on lines 16 to 17
// FIXME: How do I use a BOSD Descriptor here?
pub destination: B256,
Copy link
Contributor

Choose a reason for hiding this comment

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

Vec<u8> still, we can avoid validating it until we actually use it and take advantage of the simpler representation elsewhere.

@storopoli storopoli force-pushed the STR-754 branch 5 times, most recently from 6bc6c21 to 5e25a11 Compare January 14, 2025 18:47
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.

2 participants