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

doc: Add developer documentation to isminetype #17350

Open
wants to merge 1 commit into
base: master
from

Conversation

@HAOYUatHZ
Copy link

HAOYUatHZ commented Nov 2, 2019

Closes: #17217

src/wallet/ismine.h Outdated Show resolved Hide resolved
@HAOYUatHZ HAOYUatHZ requested a review from laanwj Nov 3, 2019
Copy link
Member

promag left a comment

Concept ACK.

src/wallet/ismine.h Outdated Show resolved Hide resolved
@HAOYUatHZ HAOYUatHZ requested a review from promag Nov 3, 2019
ISMINE_WATCH_ONLY = 1 << 0, //!< the scriptPubKey has been imported into the wallet
ISMINE_SPENDABLE = 1 << 1, //!< the scriptPubKey is corresponding to an address owned by the wallet user (can spend with the private key)
ISMINE_USED = 1 << 2, //!< the scriptPubKey is corresponding to an reused address owned by the wallet user (can spend with the private key)
ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE, //!< ISMINE_WATCH_ONLY or ISMINE_SPENDABLE

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 4, 2019

Member

I don't think these two particular comments add anything to what is in the code. What about:
"All ISMINE flags except for USED"
"All ISMINE flags including USED"

ISMINE_NO = 0, //!< the scriptPubKey is not contained in the wallet
ISMINE_WATCH_ONLY = 1 << 0, //!< the scriptPubKey has been imported into the wallet
ISMINE_SPENDABLE = 1 << 1, //!< the scriptPubKey is corresponding to an address owned by the wallet user (can spend with the private key)
ISMINE_USED = 1 << 2, //!< the scriptPubKey is corresponding to an reused address owned by the wallet user (can spend with the private key)

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 4, 2019

Member

used, not *re-*used, after all this is part of the avoid-reuse logic 😄

This comment has been minimized.

Copy link
@laanwj

laanwj Nov 4, 2019

Member

also "(can spend with the private key)" doesn't belong here, USED doesn't imply anything about spendability in the future

HAOYUatHZ added a commit to HAOYUatHZ/bitcoin that referenced this pull request Nov 4, 2019
@HAOYUatHZ HAOYUatHZ requested a review from laanwj Nov 4, 2019
@HAOYUatHZ

This comment has been minimized.

Copy link
Author

HAOYUatHZ commented Nov 7, 2019

hi @laanwj and @promag

can u take a look again? let me know if there's any mistake, thx

Copy link
Member

jonatack left a comment

Concept ACK. A few nits below. It might be good to squash the commits.

ISMINE_NO = 0, //!< the scriptPubKey is not contained in the wallet
ISMINE_WATCH_ONLY = 1 << 0, //!< the scriptPubKey has been imported into the wallet
ISMINE_SPENDABLE = 1 << 1, //!< the scriptPubKey is corresponding to an address owned by the wallet user (can spend with the private key)
ISMINE_USED = 1 << 2, //!< the scriptPubKey is corresponding to an used address owned by the wallet user

This comment has been minimized.

Copy link
@jonatack

jonatack Nov 7, 2019

Member

Suggestion for lines 22 and 23: s/is corresponding/corresponds/

This comment has been minimized.

Copy link
@HAOYUatHZ

HAOYUatHZ Nov 13, 2019

Author

Suggestion for lines 22 and 23: s/is corresponding/corresponds/

fixed

ISMINE_ALL = ISMINE_WATCH_ONLY | ISMINE_SPENDABLE,
ISMINE_ALL_USED = ISMINE_ALL | ISMINE_USED,
ISMINE_ENUM_ELEMENTS,
ISMINE_NO = 0, //!< the scriptPubKey is not contained in the wallet

This comment has been minimized.

Copy link
@jonatack

jonatack Nov 7, 2019

Member

Suggestion: remove "contained"

This comment has been minimized.

Copy link
@HAOYUatHZ

HAOYUatHZ Nov 13, 2019

Author

Suggestion: remove "contained"

fixed

ISMINE_NO = 0, //!< the scriptPubKey is not contained in the wallet
ISMINE_WATCH_ONLY = 1 << 0, //!< the scriptPubKey has been imported into the wallet
ISMINE_SPENDABLE = 1 << 1, //!< the scriptPubKey is corresponding to an address owned by the wallet user (can spend with the private key)
ISMINE_USED = 1 << 2, //!< the scriptPubKey is corresponding to an used address owned by the wallet user

This comment has been minimized.

Copy link
@jonatack

jonatack Nov 7, 2019

Member

s/an used/a used/

Reason: "used" is pronounced as if it began with the consonant "y"

This comment has been minimized.

Copy link
@jonatack

jonatack Nov 7, 2019

Member

(See discussion here #16047 (comment))

This comment has been minimized.

Copy link
@HAOYUatHZ

HAOYUatHZ Nov 13, 2019

Author

s/an used/a used/

Reason: "used" is pronounced as if it began with the consonant "y"

fixed

doc: Add developer documentation to isminetype

fix for doxygen parsing

align indentation

fix for #17350 (comment) & #17350 (comment) & #17350 (comment)

fix for jonatack's suggestions
@HAOYUatHZ HAOYUatHZ force-pushed the HAOYUatHZ:doc_isminetype branch from efbda5f to c9b6527 Nov 13, 2019
@HAOYUatHZ

This comment has been minimized.

Copy link
Author

HAOYUatHZ commented Dec 2, 2019

Hi @laanwj
this PR has been open for a while,
does it look good to you? :D

@fanquake fanquake requested a review from MarcoFalke Dec 2, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 3, 2019

yes it looks ok to me, but someone who is closer with the wallet code should make sure it's correct

@fanquake fanquake requested a review from achow101 Dec 3, 2019
@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Dec 3, 2019

Concept meh.

I like having more documentation, but soon, the definition of some types is both going to change and have two meanings, depending on the ScriptPubKeyMan in use.

For example, ISMINE_SPENDABLE currently means that the wallet has enough private keys to produce a valid scriptSig/scriptWitness. But once we get descriptor wallets and other future ScriptPubKeyMan implementations, ISMINE_SPENDABLE just becomes that the scriptPubKey matches what we are looking for regardless of whether we are able to spend the outputs or not. However because LegacyScriptPubKeyMan will still be around, the first definition is also still correct. And in that second case, ISMINE_WATCHONLY becomes meaningless and is not used at all.

I don't think that there's really a concise way to document that inline. So we would need to have a block comment which documents the meanings under the LegacyScriptPubKeyMan model and the "future ScriptPubKeyMans" model.

@HAOYUatHZ

This comment has been minimized.

Copy link
Author

HAOYUatHZ commented Dec 3, 2019

meh

I see. So should I wait for future implementations? Will adding "future ScriptPubKeyMans" doc be confusing ATM?

@HAOYUatHZ

This comment has been minimized.

Copy link
Author

HAOYUatHZ commented Dec 5, 2019

Concept meh.

I like having more documentation, but soon, the definition of some types is both going to change and have two meanings, depending on the ScriptPubKeyMan in use.

For example, ISMINE_SPENDABLE currently means that the wallet has enough private keys to produce a valid scriptSig/scriptWitness. But once we get descriptor wallets and other future ScriptPubKeyMan implementations, ISMINE_SPENDABLE just becomes that the scriptPubKey matches what we are looking for regardless of whether we are able to spend the outputs or not. However because LegacyScriptPubKeyMan will still be around, the first definition is also still correct. And in that second case, ISMINE_WATCHONLY becomes meaningless and is not used at all.

I don't think that there's really a concise way to document that inline. So we would need to have a block comment which documents the meanings under the LegacyScriptPubKeyMan model and the "future ScriptPubKeyMans" model.

I see. So should I wait for future implementations? Will adding "future ScriptPubKeyMans" doc be confusing ATM?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Jan 20, 2020

I like having more documentation, but soon, the definition of some types is both going to change and have two meanings, depending on the ScriptPubKeyMan in use.

If you disagree with the concept of documenting isminetype, please post in #17217. It's probably better to close that in that case so that people don't waste time trying to document it in a PR that gets stuck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.