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

[VDG] Fix less option sends the same amount #10332

Merged
merged 8 commits into from Mar 30, 2023

Conversation

soosr
Copy link
Collaborator

@soosr soosr commented Mar 22, 2023

Fixes #7861

#7861 (comment):

We discussed this with @molnard. If the BnB found a solution with the same amount, only that one will be suggested. And also the text should be something more proper.

In this commit I moved a similar check to have everything in the same place.
I don't know how to test it, review it carefully.

@soosr soosr changed the title Fix 7861 same amount [VDG] Fix less option sends the same amount Mar 22, 2023
adamPetho
adamPetho previously approved these changes Mar 22, 2023
Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

tACK, only one suggestion on same amount.

image

adamPetho
adamPetho previously approved these changes Mar 24, 2023
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

It is not easy to understand the suggestion in this case if you do not pay attention to the fee.
IMO the change avoidance message should somehow emphasize that we are sending the same amount but using less fees.

Shouldn't we add some unit test for this case?

@yahiheb yahiheb requested a review from kiminuo March 27, 2023 01:38
@soosr
Copy link
Collaborator Author

soosr commented Mar 27, 2023

IMO the change avoidance message should somehow emphasize that we are sending the same amount but using less fees.

In general ACK, but should be well thought out about how to display that information too without overwhelming the user. (Notice it can be Less/More/Same too).
Out of the scope of this PR as this request should also apply for less/more options.

SuperJMN
SuperJMN previously approved these changes Mar 29, 2023
Copy link
Collaborator

@SuperJMN SuperJMN Mar 29, 2023

Choose a reason for hiding this comment

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

It might be a good idea to extract out the suggestion generating algorithm outside the ViewModel. It's usually a good idea when algorithms are not quite obvious. It could also help testability.

@soosr
Copy link
Collaborator Author

soosr commented Mar 30, 2023

Shouldn't we add some unit test for this case?

The guys are working to make the ViewModels testable, there will be a test for sure once they get to this point.

…gestionViewModel.cs

Co-authored-by: yahiheb <52379387+yahiheb@users.noreply.github.com>
@soosr soosr dismissed stale reviews from SuperJMN and adamPetho via f1a6c86 March 30, 2023 14:40
@yahiheb
Copy link
Collaborator

yahiheb commented Mar 30, 2023

IMO the change avoidance message should somehow emphasize that we are sending the same amount but using less fees.

In general ACK, but should be well thought out about how to display that information too without overwhelming the user. (Notice it can be Less/More/Same too).
Out of the scope of this PR as this request should also apply for less/more options.

I created #10399 for this.

Shouldn't we add some unit test for this case?

The guys are working to make the ViewModels testable, there will be a test for sure once they get to this point.

We should keep track of these to not forget about them.

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

LGTM

@soosr soosr merged commit bac7445 into zkSNACKs:master Mar 30, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BnB - Less option sends the same amount
5 participants