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] Simplify insufficient balance handling #9508

Merged

Conversation

soosr
Copy link
Collaborator

@soosr soosr commented Nov 10, 2022

#9289 simplified a lot so the unused code can be removed.

I started with only removing the dead code but ended up with refactoring. So from now, only fee rate change can cause the exception. Previously it could be due to Pocket changing, total balance sending, and fee changing.

What is the new logic:
The user increases the transaction fee, selected amount cannot cover it anymore,

  1. Always select more coins (in the background) until it is possible.
  2. If not possible, then adjust the transaction fee to the maximum possible one.
  3. If not possible, then error.

Test cases:

  • Send your total balance (fee is subtracted from the amount)
  • Send a little bit less than your total balance (error)
  • Use the lowest fee rate, send a little bit less than the total balance, change the fee rate to the maximum. (fee rate should be adjusted)
  • Low fee rate. Send an amount which little bit less than pocket A amount, (transaction uses pocket A). Increase fee. (additional pockets should be selected to cover the fee)

@soosr
Copy link
Collaborator Author

soosr commented Nov 10, 2022

@yahiheb could you test this PR?

@soosr soosr marked this pull request as ready for review November 10, 2022 10:19
@soosr soosr marked this pull request as draft November 11, 2022 14:42
@soosr
Copy link
Collaborator Author

soosr commented Nov 11, 2022

Converted to draft, found a case that is not covered with the new solution.

@soosr soosr marked this pull request as ready for review November 24, 2022 13:21
@soosr
Copy link
Collaborator Author

soosr commented Nov 24, 2022

Ready for test and review @zkSNACKs/visual-design-group @yahiheb

Copy link
Collaborator

@ichthus1604 ichthus1604 left a comment

Choose a reason for hiding this comment

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

tACK. Code is also okay, with minimal style observations in classes such as TransactionPreviewViewModel, which are not directly related to this PR and hence will be treated separately.

Copy link
Collaborator

@SuperJMN SuperJMN left a comment

Choose a reason for hiding this comment

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

utACK.

Code looks fine. The BuildTransactionAsync method is now more readable. Good work!
Please, see the comments regarding future improvements.

@soosr soosr requested a review from molnard December 15, 2022 07:30
@soosr soosr merged commit eab97b2 into zkSNACKs:master Jan 9, 2023
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.

None yet

4 participants