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

feat: create complete-deposit sign requests #615

Merged
merged 23 commits into from
Oct 9, 2024

Conversation

djordon
Copy link
Collaborator

@djordon djordon commented Oct 3, 2024

Description

Closes #563.

Changes

  • Remove some of the unnecessary fields from the TxSignerEventLoop and TxCoordinatorEventLoop. We can remove the private key field too but it's better to move that into the wallet so I left it as is for now.
  • Removed an unnecessary input from the RotateKeysV1 transaction object.
  • Added a method for loading the SignerWallet from the database.
  • Added trait functions for fetching "fulfilled" requests from the database, partly implement one of these functions for the postgres type.

There is still some outstanding work here:

  • Rename the Fulfilled*Request structs (the requests are only partly fulfilled). Name suggestions are welcome. Done.
  • Fix-up the documentation on the Fulfilled*Request structs. Done.
  • Add an implementation for the in-memory version of the query. Won't do here.
  • Any other name suggestions are welcome. No need, unless they're really really good.

Testing Information

This PR has tests for the function that loads from the database, but is missing:

  1. Properly construct a sign deposit request transaction.
  2. Properly fetch nearly fulfilled requests from the database. For now, the query that is here is "sufficient", but we can do it properly after [Feature]: Store the requests tied to signer bitcoin transactions #585 lands.

Edit: We have some tests for (2) now, but a full suite of tests require #585 and for us to modify the query included here.

Checklist:

  • I have performed a self-review of my code

@djordon djordon added the sbtc signer binary The sBTC Bootstrap Signer. label Oct 3, 2024
@djordon djordon added this to the Minimum Working Deposit Flow milestone Oct 3, 2024
@djordon djordon changed the title feat: create complete deposit transactions feat: create complete-deposit transactions Oct 4, 2024
@djordon djordon changed the title feat: create complete-deposit transactions feat: create complete-deposit sign requests Oct 4, 2024
signer/src/block_observer.rs Show resolved Hide resolved
signer/src/stacks/wallet.rs Outdated Show resolved Hide resolved
// deposit requests, so we do not need to specify the signer set.
let test_params = crate::testing::storage::model::Params {
num_bitcoin_blocks: 10,
num_stacks_blocks_per_bitcoin_block: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit/question: num_stacks_blocks_per_bitcoin_block: 0 was confusing at the beginning, then I saw that we always include at least one stacks block in test data generate_stacks_blocks. To avoid surprises, should we validate num_stacks_blocks_per_bitcoin_block > 0 or avoid creating a stacks block if the param is zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it creates zero stacks blocks. I might be missing something, why do you think it creates one block?

Copy link
Collaborator

Choose a reason for hiding this comment

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

let stacks_blocks = (1..num_stacks_blocks).fold(vec![stacks_block], |mut blocks, _| {

It seems to always push at least one block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh, I believe (1..0) is an empty range.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it seems to be still considering the initial value though:

# dbg!((1..0).fold(vec![1], |a, _| a));
[src/main.rs:5:5] (1..0).fold(vec![1], |a, _| a) = [
    1,
]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, right, well that settles it. My mistake!

Also, I did not take in the confusing part about this test, which is that I use Stacks blocks but ask for zero stacks blocks to be generated. I'll fix this in another PR.

To avoid surprises, should we validate num_stacks_blocks_per_bitcoin_block > 0 or avoid creating a stacks block if the param is zero?

I would think that we should avoid creating Stacks blocks if we ask for zero. For this code, I should probably just update the number of stacks blocks that are needed to avoid confusion. I'll make sure that it is added in.

signer/src/stacks/wallet.rs Show resolved Hide resolved
signer/src/storage/model.rs Outdated Show resolved Hide resolved
signer/src/storage/model.rs Outdated Show resolved Hide resolved
signer/src/storage/model.rs Outdated Show resolved Hide resolved
signer/src/storage/model.rs Outdated Show resolved Hide resolved
signer/src/testing/wsts.rs Show resolved Hide resolved
signer/src/transaction_coordinator.rs Outdated Show resolved Hide resolved
@aldur aldur assigned aldur and djordon and unassigned aldur Oct 7, 2024
@djordon djordon force-pushed the 563-create-complete-deposit-transactions branch from 40100f8 to 70da07f Compare October 7, 2024 19:09
@AshtonStephens
Copy link
Collaborator

Created ticket to track the lack of integration tests here: #631

Copy link
Collaborator

@matteojug matteojug left a comment

Choose a reason for hiding this comment

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

Other than the mentioned issues in the PR description (and the two missing nits), LGTM.

About Properly construct a sign deposit request transaction. mentioned in the testing information, what are you referring to?

signer/tests/integration/postgres.rs Outdated Show resolved Hide resolved
@djordon djordon marked this pull request as ready for review October 9, 2024 13:39
@djordon
Copy link
Collaborator Author

djordon commented Oct 9, 2024

About Properly construct a sign deposit request transaction. mentioned in the testing information, what are you referring to?

I should change the wording here, I think the full sentence is something like: "test that we properly create a sign-deposit-request-transaction object" or something like that.

@djordon djordon merged commit bd0478e into main Oct 9, 2024
4 checks passed
@djordon djordon deleted the 563-create-complete-deposit-transactions branch October 9, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sbtc signer binary The sBTC Bootstrap Signer.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Feature]: Create complete-deposit transactions
4 participants