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: implement get_signer_utxo for in memory storage #567

Merged
merged 8 commits into from
Oct 1, 2024

Conversation

matteojug
Copy link
Collaborator

@matteojug matteojug commented Sep 24, 2024

Description

Initial work for #538.

I'll add the postgres impl (and more tests, if needed) in a different PR to keep this small, if there are no objections :)

Changes

  • Move get_signer_utxo to storage trait (as the issue suggest to use the database)
  • Implement get_signer_utxo for in memory db
  • Add some tests for it

Testing Information

Since we no longer mock it, should_be_able_to_coordinate_signing_rounds now also need sbtc txns to work.

Checklist:

  • I have performed a self-review of my code
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@matteojug matteojug requested review from djordon and cylewitruk and removed request for djordon September 24, 2024 23:40
Copy link
Contributor

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Nice. Gave it a quick look over just now and will have a proper review tomorrow. Here is what I have so far.

signer/src/storage/in_memory.rs Outdated Show resolved Hide resolved
signer/src/storage/in_memory.rs Outdated Show resolved Hide resolved
signer/src/storage/mod.rs Outdated Show resolved Hide resolved
signer/src/storage/mod.rs Outdated Show resolved Hide resolved
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.

Tossed in a few comments/suggestions. I didn't look too hard at the tests as I think there will be a bit of refactoring there after this comment. Good work so far!

signer/src/storage/in_memory.rs Outdated Show resolved Hide resolved
signer/src/storage/postgres.rs Outdated Show resolved Hide resolved
signer/src/testing/storage/model.rs Outdated Show resolved Hide resolved
signer/src/error.rs Outdated Show resolved Hide resolved
@cylewitruk cylewitruk added the sbtc bootstrap signer The sBTC Bootstrap Signer. label Sep 25, 2024
@cylewitruk cylewitruk linked an issue Sep 25, 2024 that may be closed by this pull request
1 task
@matteojug matteojug marked this pull request as ready for review September 26, 2024 17:23
Copy link
Contributor

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Looks good.

I think the logic looks sound. But can we add another test for the case where we have a fork with chains of the same length and UTXOs on both? You can either modify an existing test or add a new one.

signer/src/storage/in_memory.rs Outdated Show resolved Hide resolved
signer/src/testing/transaction_coordinator.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@djordon djordon left a comment

Choose a reason for hiding this comment

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

Looks good ✅

@matteojug matteojug merged commit a6f70f7 into main Oct 1, 2024
4 checks passed
@matteojug matteojug deleted the feat/get-signer-utxo branch October 1, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sbtc bootstrap signer The sBTC Bootstrap Signer.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Feature]: Get signers' UTXO
3 participants