-
Notifications
You must be signed in to change notification settings - Fork 257
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
Replace send-to-self with dual send+receive entries #119
Conversation
Concept ACK. |
@luke-jr what's the difference between raising a pull on this repository vs the bitcoin repository (as per your original 15115 pull request)? Sorry to ask this here but I'm unsure of where best to ask this. |
Moving into the GUI repo was requested explicitly by @fanquake. |
Concept ACK |
Rebased, ping @Rspigler @promag @Empact @jonasschnelli |
Will build/test tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK and light code review aa744e4. Will post full review.
Rebased |
Getting:
when trying to open Edit: I'm getting this on master too |
cc @achow101 |
ACK 2bb4e30 Tested that it does indeed split up send-to-self payments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested 2bb4e30 on Ubuntu 22.04:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first commit 880b151 fails to compile:
qt/transactionrecord.cpp: In static member function ‘static QList<TransactionRecord> TransactionRecord::decomposeTransaction(const interfaces::WalletTx&)’:
qt/transactionrecord.cpp:46:22: error: ‘ISMINE_NO’ was not declared in this scope; did you mean ‘wallet::ISMINE_NO’?
46 | fAllFromMe = ISMINE_NO;
| ^~~~~~~~~
| wallet::ISMINE_NO
In file included from qt/transactionrecord.cpp:10:
./wallet/ismine.h:42:5: note: ‘wallet::ISMINE_NO’ declared here
42 | ISMINE_NO = 0,
| ^~~~~~~~~
@luke-jr Are you planning to address #119 (review)? |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
…g whether a transaction is a send or receive This changes behaviour (IMO for the better) in the case where some but not all inputs are from us, and the net amount is positive.
If we didn't send it, it can't possibly be change, even if that's the key's purpose
Fixed. Remaining commits are identical. |
else | ||
{ | ||
isminetype fAllToMe = ISMINE_SPENDABLE; | ||
if (fAllFromMe || !any_from_me) { | ||
for (const isminetype mine : wtx.txout_is_mine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In f3fbe99 "GUI: TransactionRecord: Refactor to turn send-to-self into send+receive pairs"
This for loop seems to be duplicated from the one above. I think this could be refactored so that we don't need to duplicate this work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 099dbe4.
Going to merge it, leaving further improvements for follow-ups.
Needed to support vector<bool> txout_is_change field added in bitcoin-core/gui#119
Needed to support vector<bool> txout_is_change field added in bitcoin-core/gui#119
1c6fb04 Add support for vector<bool> serialization (Ryan Ofsky) Pull request description: Needed to support `vector<bool>` `txout_is_change` field added in bitcoin-core/gui#119 Top commit has no ACKs. Tree-SHA512: e7b87a6c52c09631c6effef66f79fa2954e8e5a6e4d17f348dc7295d6c3bd13da29922bd8802dcf5d566c2c72dcfed37da6889cc733850c8479bdd3113e2f6c9
Makes the GUI transaction list more like the RPC, and IMO clearer in general.
As a side effect, this also fixes the GUI entries when a transaction is a net profit to us, but some inputs were also from us.
Originally bitcoin/bitcoin#15115
Has Concept ACKs from @*Empact @*jonasschnelli