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: emily get transactions by input address #1294

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

Jiloc
Copy link
Collaborator

@Jiloc Jiloc commented Jan 31, 2025

Description

Closes: #1218

This PR builds on top of #1197. Most of the changes are autogenerated code.

Changes

  • add new https://{base_url}/deposit/reclaim-pubkey/{pubkey} endpoint. Follows the same approach as feat: Query By Recipient #1125 adding a new secondary global index to DynamoDB.
  • the reclaim pubkey is extracted by the reclaim_script.
    • Currently it only supports the reclaim script generated by the bridge and leather

Testing Information

  • add unit tests and adapt integration tests

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

@Jiloc Jiloc self-assigned this Feb 3, 2025
@Jiloc Jiloc added the emily API that communicates with Signers to trigger sBTC operations. label Feb 3, 2025
@Jiloc Jiloc added this to the sBTC: Deposits milestone Feb 3, 2025
@Jiloc Jiloc marked this pull request as ready for review February 3, 2025 19:21
@Jiloc Jiloc requested review from djordon and cylewitruk February 3, 2025 19:22
@Jiloc Jiloc requested a review from MCJOHN974 February 3, 2025 19:22
Copy link
Collaborator

@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.

Hmmm, we might need to find a new way here, since I just realized that we do not have the input address/scriptPubKey in the transaction. I'm not sure anything can be done without reaching out to bitcoin-core.

Comment on lines 156 to 162
let input_address = {
let params = if is_mainnet {
Params::MAINNET
} else {
Params::REGTEST
};
Address::from_script(&txin.script_sig, params.clone())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure that we want to use Address here instead of the scriptPubKey? I feel like we should shy away from addresses if possible and just use the scriptPubKey. That's what is in the transaction (when it is an output) so it seems most natural to use that and encode it as hex.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, sorry for looking at this too late, but we might not be able to use either the scriptPubKey or the "address" because we do not have them (they're never included in the transaction when they're being spent). This scriptSig is not the same as the scriptPubKey from the transaction output, it's basically an unlocking script that, when paired with the actual scriptPubKey, allows you to spend the funds. It's often empty too. For example, all deposit request outputs and all signer UTXOs have empty scriptSigs when spent in a sweep transaction. It's basically only used for legacy scriptPubKeys like P2PKH. I think that the only option is to abandon the endeavor or use bitcoin core for the information.

Also, since a transaction's first input does not necessarily map to a "definitive" source address for the deposit, we may need to make some other changes here too. For example, we might want to check all input scriptPubKeys and using that if they all match. Or maybe map all input scriptPubKeys to the same deposit. All of this is assuming we can get the scriptPubKey at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Man this is very appreciated! A very simple alternative would be to add an address_hex field in the create_deposit request itself. I am discussing it with @hozzjss and @setbern

Copy link
Collaborator

Choose a reason for hiding this comment

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

Man this is very appreciated! A very simple alternative would be to add an address_hex field in the create_deposit request itself. I am discussing it with @hozzjss and @setbern

No problem 😄.

One thing to consider is how to verify that information if we include it. I couldn't think of a way without having the input transactions or bitcoin core (or a trusted equivalent).

@Jiloc Jiloc requested a review from djordon February 5, 2025 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
emily API that communicates with Signers to trigger sBTC operations.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Design: How will the bridge get the history for transaction
2 participants