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.

Sign up for GitHub

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

Jump to bottom

feat: add validation for accept-withdrawal-request transactions #555

Merged
merged 26 commits into from
Sep 24, 2024
Merged

feat: add validation for accept-withdrawal-request transactions #555

merged 26 commits into from
Sep 24, 2024

Conversation

djordon
Copy link
Contributor

@djordon djordon commented Sep 21, 2024

Description

Closes #477.

Changes

The changes here are very similar in spirit to the changes in #545.

  • Add a SignerVotes type for converting to a bitmap
  • Implement validate for the AcceptWithdrawalV1 type, which is used for accept-withdrawal-request contract calls.
  • Minor clean-up of the integration tests for the complete-deposit validation integration tests.

Testing Information

This PR adds integration tests. There is still some work to what is here before it is finalize, specifically #552 and #554.

Checklist:

  • I have performed a self-review of my code

@djordon
Make the serde implementations transparent
bab7244
@djordon
Change the name of a struct
1691c8f
@djordon
Add validation to the CompleteDepositV1 struct.
3862c35 
Update the number of arguments that it needs.
Update the trait validation function to take in a context object.
@djordon
Add integration tests to the validation function
a7c6575
@djordon
Update a stray comment
528fde2
@djordon
This makes more sense
00e5a39
@djordon
minor clean-ups
5d462c0
@djordon
Update comments, fix typos
3f4e6c4
@djordon
Fix more typos
1f1197c
@djordon
Merge branch 'main' into 479-validate-complete-deposit-sign-requests
5295570
@djordon
cargo fmt
ae8f00b
@djordon
Fix typo, and simplify error message variant names
ae6ed9e
@djordon
Add a todo
531727d
@djordon
cargo fmt
80c010f
@djordon
Add a SignerVotes type for converting to a bitmap
5c1a27c
@djordon
Add an aggregate key field to the
59f1b9d 
request context object
@djordon
Implement validate for the AcceptWithdrawalV1
6ee9ab2 
struct
@djordon
Minor clean-up of existing tests
c022f21
@djordon
add integration tests for the validation logic
817ea3f
@djordon djordon added the sbtc bootstrap signer The sBTC Bootstrap Signer. label Sep 21, 2024
@djordon djordon mentioned this pull request Sep 21, 2024
Open
2 tasks
@AshtonStephens AshtonStephens added this to the sBTC Code Complete milestone Sep 23, 2024
cylewitruk
cylewitruk reviewed Sep 23, 2024
View reviewed changes
Copy link
Member

@cylewitruk cylewitruk left a comment

Choose a reason for hiding this comment

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

Haven't gotten all the way through yet but here's my comments so far :)

signer/src/bitcoin/utxo.rs Show resolved Hide resolved
signer/src/stacks/contracts.rs Show resolved Hide resolved
signer/src/stacks/contracts.rs Show resolved Hide resolved
signer/src/stacks/contracts.rs Show resolved Hide resolved
signer/src/stacks/contracts.rs Show resolved Hide resolved
signer/src/error.rs Show resolved Hide resolved
@djordon djordon mentioned this pull request Sep 23, 2024
Merged
1 task
cylewitruk
cylewitruk reviewed Sep 24, 2024
View reviewed changes
Copy link
Member

@cylewitruk cylewitruk left a comment

Choose a reason for hiding this comment

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

Still have a few tests left to go through but here's a few more comments (same stuff as on the other PR pretty much)

signer/tests/integration/withdrawal_accept.rs Outdated Show resolved Hide resolved
signer/tests/integration/withdrawal_accept.rs Outdated Show resolved Hide resolved
signer/tests/integration/withdrawal_accept.rs Outdated Show resolved Hide resolved
Base automatically changed from 479-validate-complete-deposit-sign-requests to main September 24, 2024 05:12
cylewitruk
cylewitruk reviewed Sep 24, 2024
View reviewed changes
Copy link
Member

@cylewitruk cylewitruk left a comment

Choose a reason for hiding this comment

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

Alright, finally got through this -- few docs suggestions and one wrong test doc i think. Otherwise looks good :)

signer/tests/integration/withdrawal_accept.rs Outdated Show resolved Hide resolved
signer/tests/integration/withdrawal_accept.rs Show resolved Hide resolved
signer/tests/integration/withdrawal_accept.rs Show resolved Hide resolved
signer/tests/integration/withdrawal_accept.rs Outdated Show resolved Hide resolved
cylewitruk
cylewitruk approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@cylewitruk cylewitruk left a comment

Choose a reason for hiding this comment

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

lgtm 🥳

@djordon djordon merged commit 10b150d into main Sep 24, 2024
4 checks passed
@djordon djordon deleted the 477-validate-accept-withdrawal-request-contract-calls branch September 24, 2024 16:47
This was referenced Sep 24, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cylewitruk cylewitruk cylewitruk approved these changes

@matteojug matteojug Awaiting requested review from matteojug

@AshtonStephens AshtonStephens Awaiting requested review from AshtonStephens

Assignees
No one assigned
Labels
sbtc bootstrap signer The sBTC Bootstrap Signer.
Projects
None yet
Milestone
sBTC MVP Code Complete
Development

Successfully merging this pull request may close these issues.

[Feature]: Validate accept-withdrawal-request contract calls
3 participants
@djordon @cylewitruk @AshtonStephens