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

Document return type of sign method. #834

Merged
merged 1 commit into from Jan 31, 2023

Conversation

Tibo-lg
Copy link
Contributor

@Tibo-lg Tibo-lg commented Jan 13, 2023

Small thing but I had to look up the code to see what the returned boolean was about, thought it might be good to have in the docs.

Description

Documents the meaning of the returned value on the wallet sign method.

I've just edited from github so skipped all the checks if that's not ok maybe someone else can update it properly.

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

Approach ACK

@@ -1084,7 +1084,7 @@ where
}

/// Sign a transaction with all the wallet's signers, in the order specified by every signer's
/// [`SignerOrdering`]
/// [`SignerOrdering`]. Returns true if the PSBT was finalized, and false otherwise.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This function returns the Result type with an encapsulated bool that has the value true if the PSBT was finalized, or false otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@notmandatory
Copy link
Member

This looks good, but it needs a rebase since we changed our CI MSRV.

@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Jan 30, 2023

Rebased on master but someone needs to approve the CI run.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK bf8b0f9

@notmandatory
Copy link
Member

notmandatory commented Jan 30, 2023

Thanks @Tibo-lg for the rebase, but I also see that your commit needs to be signed. Can you setup your git signing and rebase your commit again to get it signed?

Small thing but I had to look up the code to see what the returned boolean was about, thought it might be good to have in the docs.
@Tibo-lg
Copy link
Contributor Author

Tibo-lg commented Jan 31, 2023

Thanks @Tibo-lg for the rebase, but I also see that your commit needs to be signed. Can you setup your git signing and rebase your commit again to get it signed?

Done!

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

ACK ff72078

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK ff72078

@notmandatory notmandatory merged commit 97f8fe3 into bitcoindevkit:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants