Skip to content

STR-406 Implement deposit duty and other RPCs #320

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

Merged

Conversation

Rajil1213
Copy link
Contributor

@Rajil1213 Rajil1213 commented Sep 25, 2024

Description

This PR:

  • Adds a db method to get all txs in the L1Db after a certain index.
  • Adds an RPC to get all deposit duties after a certain index.
  • Adds the bitcoind_network as a field in AlpenRpcImpl (this might not be the right place for it).
  • Refactors out the Error enum from the sequencer/rpc_server and introduces an extractor module to make it slightly more manageable/testable.

TODO

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

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

Closes STR-406
Closes STR-428

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 76.62835% with 122 lines in your changes missing coverage. Please review.

Project coverage is 61.08%. Comparing base (864c8d1) to head (e9eab54).
Report is 21 commits behind head on master.

Files with missing lines Patch % Lines
sequencer/src/rpc_server.rs 0.00% 43 Missing ⚠️
crates/rpc/types/src/errors.rs 0.00% 28 Missing ⚠️
sequencer/src/extractor.rs 93.81% 19 Missing ⚠️
crates/state/src/bridge_state.rs 0.00% 12 Missing ⚠️
crates/primitives/src/buf.rs 0.00% 6 Missing ⚠️
crates/state/src/bridge_duties.rs 0.00% 6 Missing ⚠️
crates/rocksdb-store/src/l1/db.rs 95.45% 5 Missing ⚠️
crates/bridge-tx-builder/src/deposit.rs 0.00% 1 Missing ⚠️
crates/rpc/types/src/types.rs 0.00% 1 Missing ⚠️
crates/tx-parser/src/deposit/deposit_request.rs 85.71% 1 Missing ⚠️
@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
+ Coverage   60.61%   61.08%   +0.46%     
==========================================
  Files         211      213       +2     
  Lines       22587    23033     +446     
==========================================
+ Hits        13692    14070     +378     
- Misses       8895     8963      +68     
Files with missing lines Coverage Δ
crates/db/src/traits.rs 100.00% <ø> (ø)
crates/primitives/src/errors.rs 100.00% <ø> (+100.00%) ⬆️
crates/rpc/api/src/lib.rs 0.00% <ø> (ø)
crates/test-utils/src/bridge.rs 95.48% <100.00%> (-0.03%) ⬇️
sequencer/src/main.rs 0.00% <ø> (ø)
crates/bridge-tx-builder/src/deposit.rs 95.81% <0.00%> (ø)
crates/rpc/types/src/types.rs 0.00% <0.00%> (ø)
crates/tx-parser/src/deposit/deposit_request.rs 98.83% <85.71%> (-0.57%) ⬇️
crates/rocksdb-store/src/l1/db.rs 97.29% <95.45%> (-0.91%) ⬇️
crates/primitives/src/buf.rs 74.86% <0.00%> (-2.60%) ⬇️
... and 5 more

... and 2 files with indirect coverage changes

@Rajil1213 Rajil1213 force-pushed the STR-406-Implement-deposit-duty-and-misc-RPCs-for-bridge-client branch from 6dfffff to 2042d44 Compare September 25, 2024 18:37
@Rajil1213 Rajil1213 self-assigned this Sep 25, 2024
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.

Some organizational comments.

@Rajil1213 Rajil1213 force-pushed the STR-406-Implement-deposit-duty-and-misc-RPCs-for-bridge-client branch 2 times, most recently from 37851f9 to db6bc1f Compare September 26, 2024 09:41
@Rajil1213 Rajil1213 force-pushed the STR-406-Implement-deposit-duty-and-misc-RPCs-for-bridge-client branch from db6bc1f to e1364a5 Compare September 26, 2024 19:03
@storopoli
Copy link
Member

@Rajil1213 since you're touching that part of the codebase sequencer/src/rpc_server.rs, can you incorporate https://alpenlabs.atlassian.net/browse/STR-428 once #293 is merged?

@Rajil1213
Copy link
Contributor Author

since you're touching that part of the codebase sequencer/src/rpc_server.rs, can you incorporate https://alpenlabs.atlassian.net/browse/STR-428 once #293 is merged?

That shouldn't take long. Will do that tonight and hopefully get this one merged so that it unblocks some of the work in the bridge client binary.

CC: @delbonis

@Rajil1213
Copy link
Contributor Author

@delbonis should I also handle the withdrawal duty RPC in this PR now that #315 has been merged?

@Rajil1213 Rajil1213 changed the title STR-406 Implement deposit duty RPC STR-406 Implement deposit duty and other RPC Sep 26, 2024
@Rajil1213 Rajil1213 changed the title STR-406 Implement deposit duty and other RPC STR-406 Implement deposit duty and other RPCs Sep 26, 2024
@Rajil1213 Rajil1213 force-pushed the STR-406-Implement-deposit-duty-and-misc-RPCs-for-bridge-client branch 2 times, most recently from 48b1a9b to a7684ed Compare September 26, 2024 21:09
@delbonis
Copy link
Contributor

delbonis commented Sep 27, 2024

@Rajil1213 No we can wait on that, although there should be a functional test for handling deposits. Could split out fn_bridge_deposit_happy and fn_bridge_deposit_reclaim from STR-418 into another story and do it here. The withdrawal tests can happen after.

@delbonis
Copy link
Contributor

Ah wait but crap that's blocked on the other PR too.

@Rajil1213 Rajil1213 force-pushed the STR-406-Implement-deposit-duty-and-misc-RPCs-for-bridge-client branch 3 times, most recently from e93b954 to bb597a5 Compare September 27, 2024 09:23
@Rajil1213
Copy link
Contributor Author

@storopoli I think this needs a final round of reviews and then it can be merged. The deposit duty rpc is there so the bridge client binary can work off of that. The withdrawal would be similar and will be done via a separate PR.

I've updated some of the inner workings in this PR so that:

  • The same key can be used for signing both the bridge messages and transactions.
  • The pubkey in the pubkey table is assumed to be a taproot pubkey with even parity so we must make sure that the xpriv always has a corresponding pubkey with an even parity.
  • Removed the redundant AlpenBridgeApi trait.

@Rajil1213 Rajil1213 dismissed delbonis’s stale review September 27, 2024 09:25

Dismissing stale review based on latest changes.

delbonis
delbonis previously approved these changes Sep 27, 2024
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.

If the tests are sorted out then I think we can merge this right?

@Rajil1213 Rajil1213 dismissed stale reviews from delbonis and storopoli via c41d68e September 27, 2024 19:14
@Rajil1213 Rajil1213 force-pushed the STR-406-Implement-deposit-duty-and-misc-RPCs-for-bridge-client branch from 85cd7bc to c41d68e Compare September 27, 2024 19:14
@Rajil1213 Rajil1213 force-pushed the STR-406-Implement-deposit-duty-and-misc-RPCs-for-bridge-client branch from c41d68e to e9eab54 Compare September 27, 2024 19:30
Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK
e9eab54

@Rajil1213 Rajil1213 merged commit 07a11f3 into master Sep 27, 2024
17 checks passed
@storopoli storopoli deleted the STR-406-Implement-deposit-duty-and-misc-RPCs-for-bridge-client branch September 27, 2024 20:30
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