-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Specify Amazon eGift Card country #5117
Conversation
Thanks for this, I think the payment info screen at the bottom of the trade window makes this really clear for users. |
Maybe it need also a comment that the trader can remove the payment account only if there is no open trade or offer with it. |
Moved to draft while I implement suggestion from @chimp1984. Using #4481 as reference. Also noticed that the new field has to be excluded from trade contract, e.g. bisq/core/src/main/java/bisq/core/payment/payload/RevolutAccountPayload.java Lines 56 to 60 in 1a8aba8
|
@chimp1984 I don't know how to make this change compatible with prior versions. When both clients are new:
Here's the problem:
What's the best way to resolve this? |
Hm... that is always tricky. I think with Revolut it was similar but there was some solution. I would need to dig deeper to figure it out. I think excluding it from json was the solution...It is not a critivcal field so I don't see any problem to not have that in the contract. From RevolutAccountPayload:
|
Account age is another area to take care of, but that data should not be part of the hash anyway. |
If the field is excluded from contract, the functionality does not work. The buyer needs to be shown the seller's country so that they buy the amazon eGift from the appropriate country-based website. |
The payment account data are exchanged in the trade messages so the peer gets all data. We use a hash of the contract as json and that causes the problems when adding a new field, but with json exclude that is fixed. |
Thanks @chimp1984, excluding the field from JSON resolved it again. For some reason during testing I thought it was missing on new-new clients but now its fine so I'll just chalk it off to me getting confused. 😕 |
Adds countryCode to AmazonGiftCardAccountPayload Account upgrade done at startup => Eurozone accounts will prompt for country. Trade buyer step 2 prompts use of the appropriate Amazon website for buying gift card.
Testing summary:Check that old Bisq can trade with new Bisq.
Check that account upgrade works for each possible currency:
Check that new Bisq can trade with new Bisq. Same procedure as first test, but with both instances running this PR (new) Bisq. |
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.
NACK - Trading does work across versions and also the upgrading does work as expected. The only thing I noticed is that if you are establishing a trade between an old an a new Amazon account the current client is seeing two empty fields which might leave the user confused what to do (see buy giftcard at
and country
). I think in that case we should advise the user to contact the peer in the trader chat to discuss on which country store the egift card has to be purchased at.
@ripcurlx requested change made. When seller is old, buyer will be prompted to use Trader Chat: |
Fixes #5098
[update] Changed implementation per suggestion from @chimp1984. Now it upgrades the existing accounts at startup the same way that the upgrade to Revolut was done. Accounts that are non-EUR are upgraded automatically, EUR accounts prompt the user to choose the Eurozone country:
Example: A French person buying BTC from German person.