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

dcr: Add mix split tx identification. #3061

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

JoeGruffins
Copy link
Member

closes #3057

@JoeGruffins JoeGruffins marked this pull request as ready for review November 6, 2024 10:01
@dev-warrior777
Copy link
Contributor

dev-warrior777 commented Nov 6, 2024

It is a good fix already .. but ..

"This dumbly checks for at least three outputs of the same size and three of other sizes."

How could it be less naive?

@JoeGruffins
Copy link
Member Author

How could it be less naive?

dcrdata does this https://github.com/decred/dcrdata/blob/e9c73122122a65fbb2019eb31a526a854e927edc/txhelpers/cspp.go#L84

So we could do that. However as I said in the comment we can't be sure of the transaction fee unless it is default, which it probably is, or if we also look at the ticket transaction, which may not be available yet and takes more code to find.

We could just use the base ticket price which is in the header for that block, and also look for values that are just a little bit more. However we aren't using the header yet in idUnknownTx so it's more code again.

afaik having three outputs that are the same, and then three more, would never happen naturally with what the current bisonw does. Even if we wanted to make three split outputs, then we would only have one change output.

client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
Comment on lines 6213 to 6218
if owns {
mixedAmount += uint64(txOut.Value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this also count the value of a change output? How about changing the isMixedSplitTx signature to also return the ticketOutAmt as it does in dcrdata. Then you could reuse the same block above where it checks if txOut.Value == mixDenom { .

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I wasn't sure what we wanted to show. If just ticket amounts can do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the inputs of these transactions are all the size of a mix output or a ticket redemption right? And the outputs are either the size of a ticket purchase or a change, is this correct? If so, then I think it makes sense to display the ticket amounts.

@martonp
Copy link
Contributor

martonp commented Nov 7, 2024

You could do this to avoid the duplication: 29ee36d

@JoeGruffins
Copy link
Member Author

Ok added those changes!

@buck54321 buck54321 merged commit 365525e into decred:master Nov 8, 2024
5 checks passed
buck54321 pushed a commit to buck54321/dcrdex that referenced this pull request Nov 12, 2024
* dcr: Add mix split tx identification.

* Combine regular mix and split ticket mix logic

---------

Co-authored-by: martonp <marci93@gmail.com>
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.

dcr/ui: Mixing transacion still shown when purchasing tickets.
4 participants