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

wallet: clarify replace fields in help output #27782

Closed

Conversation

torkelrogstad
Copy link
Contributor

Closes #27781.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 30, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK brunoerg, pinheadmz

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27307 (wallet: track mempool conflicts with wallet transactions by ishaanam)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@brunoerg
Copy link
Contributor

I suppose that changing them in TransactionDescriptionString will also affect listsinceblock and listtransactions. So, I think would be appropriate to update the PR title/commit message?

Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

Concept ACK

@bitcoin bitcoin deleted a comment from Macoophonic May 31, 2023
@torkelrogstad torkelrogstad changed the title wallet: clarify replace fields in gettransaction wallet: clarify replace fields in help output Jun 1, 2023
@torkelrogstad
Copy link
Contributor Author

Thanks for pointing that out @brunoerg. Updated the commit message and PR title.

@luke-jr
Copy link
Member

luke-jr commented Jun 24, 2023

otoh, it would be nice to populate these when we detect a replaced receive tx...

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

concept ACK

Included a nit for better consistency with the messages. I'll also point out, regarding the original issue, that gettransaction also returns a walletconflicts field which is present for both senders and receivers.

{RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "The txid if this tx was replaced."},
{RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "The txid if the tx replaces one."},
{RPCResult::Type::STR_HEX, "replaced_by_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx was replaced."},
{RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if the tx replaces one."},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if the tx replaces one."},
{RPCResult::Type::STR_HEX, "replaces_txid", /*optional=*/true, "Only if 'category' is 'send'. The txid if this tx replaces another."},

@brunoerg
Copy link
Contributor

Are you still working on it?

@maflcko
Copy link
Member

maflcko commented Nov 13, 2023

Closing as up for grabs for now, due to lack of reply

@maflcko maflcko closed this Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unclear documentation about TX replacements in gettransaction
6 participants