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

Implement rounding options for "Pay-By-Mail" payment type. #1059

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wp07e
Copy link
Contributor

@wp07e wp07e commented Jun 21, 2024

#1014

Adds a combo box with rounding options to the nearest integers 1, 5, 10 or 20.

Tested on 1.0.7 seems to be working fine.

All other relevant totals (i.e. Amount of XMR to sell, Min amount of XMR) are also updated as well.

There is one slight issue where the new combo box seems to re-render out-of-order so I will prob. submit another commit to resolve that but its not a functional issue.

@wp07e wp07e requested a review from woodser as a code owner June 21, 2024 08:36
@woodser
Copy link
Contributor

woodser commented Jun 21, 2024

Issues observed:

  • The rounding pull down needs a label, which uses in-built translations (can include default english to start):
image
  • The pulldown should be disabled after the next button is clicked:
image
  • Editing the offer does not update the rounded amount.

@wp07e
Copy link
Contributor Author

wp07e commented Jul 1, 2024

Ready for review.

I considered various use cases and went through multiple scenarios including:

  1. Creating multiple different offers with same / different payment account.
  2. Checking Edit screen.
  3. Canceling offer.
  4. Completing trades on testnet.

@woodser
Copy link
Contributor

woodser commented Jul 1, 2024

Looks like the in-built tests need updated. You should observe the errors locally by running make.

@woodser
Copy link
Contributor

woodser commented Jul 1, 2024

Unless you're in some weird state, you should be able to simply git reset --soft <commit hash before first commit for this pr> then git push --force to squash all commits to one.

@wp07e wp07e force-pushed the 1014_CBM_rounding branch from 2f7f125 to 5af9a9a Compare July 1, 2024 13:54
@wp07e
Copy link
Contributor Author

wp07e commented Jul 1, 2024

Thanks

NodeAddress arbitrator_signer = 36;
bytes arbitrator_signature = 37;
repeated string reserve_tx_key_images = 38;
int32 round_to = 11;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't change the numbering of existing persistent types, so this should be appended to the end and the other fields unchanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to the end and resolved issues.

@wp07e wp07e force-pushed the 1014_CBM_rounding branch 3 times, most recently from ea2fb5d to 2a4d224 Compare July 2, 2024 06:23
@wp07e wp07e force-pushed the 1014_CBM_rounding branch from 2a4d224 to c8b4f65 Compare July 2, 2024 21:45
@woodser
Copy link
Contributor

woodser commented Jul 10, 2024

The pull down should only appear when applicable for the offer type:

image

Getting this error when making offer for BCH (probably all other non fiat types):

java.lang.NullPointerException: Cannot invoke "java.lang.Integer.intValue()" because "roundTo" is null
	at haveno.core.offer.CreateOfferService.createAndGetOffer(CreateOfferService.java:223)
	at haveno.desktop.main.offer.MutableOfferDataModel.createAndGetOffer(MutableOfferDataModel.java:286)
	at haveno.desktop.main.offer.MutableOfferViewModel.createAndGetOffer(MutableOfferViewModel.java:1139)
	at haveno.desktop.main.offer.MutableOfferView.onPlaceOffer(MutableOfferView.java:354)
	at haveno.desktop.main.offer.MutableOfferView.lambda$addFundingGroup$42(MutableOfferView.java:1213)
	at javafx.base/com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:86)

@wp07e
Copy link
Contributor Author

wp07e commented Jul 12, 2024

@woodser I addressed both of the issues you mentioned and I re-tested with normal and non-traditional (i.e. BCH) and everything seems to be working. If possible, please double-check thank you.

@wp07e
Copy link
Contributor Author

wp07e commented Jul 12, 2024

@woodser Also, I think I want to try a ticket for the backend and was looking for a straight-forward, fairly easy one to start with. Do you think #1115 is a good candidate? If not, which one would you recommend to start with? No guarantees I will work on anything but just in case I decide to do it :)

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.

2 participants