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

Add docs to the 'wallet' module #223

Merged
merged 1 commit into from
Dec 16, 2020

Conversation

notmandatory
Copy link
Member

No description provided.

Copy link

@tobin-crypto tobin-crypto left a comment

Choose a reason for hiding this comment

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

I just reviewed the English (too new around here to deeply review the content).

@@ -114,8 +114,9 @@ pub type DefaultCoinSelectionAlgorithm = BranchAndBoundCoinSelection;
#[cfg(test)]
pub type DefaultCoinSelectionAlgorithm = LargestFirstCoinSelection; // make the tests more predictable

// Base weight of a Txin, not counting the weight needed for satisfaying it.
// prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) + script_len (1 bytes)
/// Base weight of a Txin, not counting the weight needed for satisfaying it

Choose a reason for hiding this comment

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

Looks like you don't have spell check enabled in your editor, you should be able to configure your editor to spell check just the comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch, I have spell checking on but I guess I missed the warning on that one. Fixed now.

@notmandatory notmandatory force-pushed the doc_fixes_wallet branch 2 times, most recently from 51023c6 to a2582bb Compare December 14, 2020 04:07
@notmandatory notmandatory marked this pull request as ready for review December 14, 2020 05:15
// prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) + script_len (1 bytes)
/// Base weight of a Txin, not counting the weight needed for satisfying it
///
/// prev_txid (32 bytes) + prev_vout (4 bytes) + sequence (4 bytes) + script_len (1 bytes)
pub const TXIN_BASE_WEIGHT: usize = (32 + 4 + 4 + 1) * 4;
Copy link
Member

Choose a reason for hiding this comment

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

I think we could make this private (like with pub(crate)), since it's only used internally

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense TXIN_BASE_WEIGHT should be pub(crate) but that made it out of scope for the example code and I couldn't find a way to make it visible. As a work around I put a (hidden) duplicate const in the example. Hopefully this is OK or maybe someone knows a better way to fix it?

PkHash(hash160::Hash),
/// The first 32 bits of the bitcoin HASH160 hash of an ECDSA public key
Copy link
Member

Choose a reason for hiding this comment

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

The fingerprint is actually the BIP32 key fingerprint, it's not the hash of a key.

I would just document this as The fingerprint of a BIP32 extended key

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.. I was a little too proud of myself for looking up the full definition. :-)

TimeoutError,
/// Invalid fingerprint in HD key path
InvalidScript,
Copy link
Contributor

@eupn eupn Dec 15, 2020

Choose a reason for hiding this comment

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

Probably a side note, but judging by the comment, shouldn't it be InvalidFingerprint instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally put his doc because it was the situation I found this variant used in a TestValidator. But I think down the road it could be used for other AddressValidatorError::InvalidScript reasons, so I just changed the docs back to match the name.

Copy link
Member

@afilini afilini left a comment

Choose a reason for hiding this comment

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

ACK 148e8c6

@afilini afilini merged commit 5f37318 into bitcoindevkit:master Dec 16, 2020
@notmandatory notmandatory mentioned this pull request Dec 16, 2020
20 tasks
@notmandatory notmandatory deleted the doc_fixes_wallet branch December 24, 2020 04:56
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.

4 participants