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

Create ContractSignerProvider #188

Merged
merged 1 commit into from
Feb 5, 2024
Merged

Conversation

benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Dec 12, 2023

Closes #185

Still some cleanup to do but curious on direction here. Tried to mimic LDK's trait as best I could.

Instead of creating a create_adoptor_signature function I went with just get_secret_key as this would be really invasive and require refactor things in other crates that weren't just dlc_manager. Also kept the old functions around for the channel handling, a lot of that I don't fully know what is going on and this felt simpler for now.

@benthecarman benthecarman changed the title Change Signer::sign_tx_input to Signer::sign_psbt_input Create ContractSignerProvider Dec 12, 2023
Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

Just had a quick look but the overall direction matches with what I had in mind. I feel it might be better to keep the signers in memory though (and maybe have the lazily created) rather than re-deriving them every time they are needed.

dlc-manager/src/lib.rs Outdated Show resolved Hide resolved
@benthecarman benthecarman force-pushed the signer branch 2 times, most recently from a7f891e to 9da1619 Compare December 18, 2023 22:42
@benthecarman
Copy link
Contributor Author

Added a cache and reworked some the APIs to be cleaner, think tests should be passing now too.

@benthecarman benthecarman force-pushed the signer branch 4 times, most recently from f822a61 to f0f7e74 Compare December 22, 2023 04:44
@benthecarman benthecarman force-pushed the signer branch 2 times, most recently from 451b25e to 8c08202 Compare January 13, 2024 00:09
@benthecarman
Copy link
Contributor Author

Got pretty much everything passing and in a clean state. Only test failing is pnl_compute_test because the serialization of OfferedContract changed. Is there an easy way to generate a new AcceptedContract for this test?

@benthecarman benthecarman requested a review from Tibo-lg January 13, 2024 10:45
@benthecarman benthecarman marked this pull request as ready for review January 13, 2024 10:46
@Tibo-lg
Copy link
Contributor

Tibo-lg commented Jan 17, 2024

Is there an easy way to generate a new AcceptedContract for this test?

Have you tried using the one within dlc-sled-storage-provider/test_files? If it works maybe it'd be best to just use that directly (update the path).

@benthecarman
Copy link
Contributor Author

Is there an easy way to generate a new AcceptedContract for this test?

Have you tried using the one within dlc-sled-storage-provider/test_files? If it works maybe it'd be best to just use that directly (update the path).

Does not work either, I imagine all of those tests are broken too :/

@Tibo-lg
Copy link
Contributor

Tibo-lg commented Jan 19, 2024

Is there an easy way to generate a new AcceptedContract for this test?

Have you tried using the one within dlc-sled-storage-provider/test_files? If it works maybe it'd be best to just use that directly (update the path).

Does not work either, I imagine all of those tests are broken too :/

Ah these ones can be generated with this script: 'scripts/generate_serialized_contract_files.sh'.

@benthecarman
Copy link
Contributor Author

Regenerated test vectors and everything seems to pass now :)

Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

Overall looks nice, but still some details to be resolved.

type Signer = SimpleSigner;

fn derive_signer_key_id(&self, _is_offer_party: bool, temp_id: [u8; 32]) -> [u8; 32] {
temp_id // fixme not safe
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this method should take more parameters, like maybe the collateral values, the event id, or whatever could be use to generate a likely unique key id. Then IIUC this should be called only once, so if using the method I mention below, you could check that the label does not exist (and maybe panic if it does?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of the parameters aren't really always available. The main problems comes from renew_offer where you have to do it from only a single party's params

bitcoin-rpc-provider/src/lib.rs Outdated Show resolved Hide resolved
bitcoin-rpc-provider/src/lib.rs Outdated Show resolved Hide resolved
dlc-manager/src/channel_updater.rs Outdated Show resolved Hide resolved
dlc-manager/src/channel_updater.rs Outdated Show resolved Hide resolved
/// Get the secret key associated with the provided public key.
///
/// Only used for Channels.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will probably need to be split in the future, but fine if you don't do it now.

dlc-manager/src/manager.rs Outdated Show resolved Hide resolved
simple-wallet/src/lib.rs Outdated Show resolved Hide resolved
simple-wallet/src/lib.rs Outdated Show resolved Hide resolved
simple-wallet/src/lib.rs Outdated Show resolved Hide resolved
@benthecarman
Copy link
Contributor Author

Think I responded to all comments

Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

Thanks, just a couple of things need fixing.

bitcoin-rpc-provider/src/lib.rs Outdated Show resolved Hide resolved
bitcoin-rpc-provider/src/lib.rs Outdated Show resolved Hide resolved
dlc-manager/src/manager.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Tibo-lg Tibo-lg left a comment

Choose a reason for hiding this comment

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

Thanks!

@Tibo-lg Tibo-lg merged commit 39893b1 into p2pderivatives:master Feb 5, 2024
69 checks passed
@benthecarman benthecarman deleted the signer branch February 5, 2024 12:11
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.

Redo secret key APIs
2 participants