-
Notifications
You must be signed in to change notification settings - Fork 492
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
[v2.0.7.2] Mix to another wallet clean #12997
[v2.0.7.2] Mix to another wallet clean #12997
Conversation
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.
tACK
I will merge this now to be able to create the RC - please still review it. It is moving the user's money. |
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.
LGTM
@@ -39,11 +39,13 @@ public async Task<CoinJoinTracker> CreateAndStartAsync(IWallet wallet, IWallet o | |||
throw new NotSupportedException("Wallet has no key chain."); | |||
} | |||
|
|||
wallet.ConsolidationMode = outputWallet is not null && outputWallet.WalletId != wallet.WalletId; |
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.
wallet.ConsolidationMode = outputWallet is not null && outputWallet.WalletId != wallet.WalletId; | |
wallet.ConsolidationMode = wallet.ConsolidationMode || (outputWallet is not null && outputWallet.WalletId != wallet.WalletId); |
?
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.
Seems to be not correct to me. Once we set the ConsolidationMode to true it won't be able to change it back. Even if the user sets it back and mixes into his own wallet.
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.
Ok so maybe another change, but right now you basically "hijack" ConsolidationMode
with this feature, as it's getting overriden, so we cannot set this settings in any other way that through mixing to other wallet.
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.
That is a correct point theoretically. Practically there is no other usage of ConsolidationMode
currently. If you know an easy way to change this we can do it that way on the master. Is it OK to have it this way in the hotfix for the sake of simplicity?
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.
Yes
fixes: #12695