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

Prevent taking of offers with unequal bank account types (excl. SEPA) #3673

Conversation

@stejbac
Copy link
Contributor

stejbac commented Nov 24, 2019

Use stricter criteria when deciding which of the taker's accounts (if any) are valid for a given offer. Specifically, prevent National Bank accounts from being used to take Same / Specific Bank(s) offers, so the three payment method types can never being mixed.

This prevents an error on the trading peer when the trade starts, due to enforcement of equal maker & taker payment method IDs (except for SEPA) in the Contract payload constructor.

This partially addresses #3602, where the erroneous peer response caused the taker to be presented with a confusing timeout, after they managed to take a SAME_BANK offer using their NATIONAL_BANK account. With this change, the taker would be directed to create a separate SAME_BANK account if they don't have one already (which would then be automatically selected).

(Later, it might be worthwhile to make a more substantial change to allow National, Same or Specific Bank account types to be mixed under certain circumstances when trading, but that could be complicated by the peer running an older version.)

stejbac added 2 commits Nov 24, 2019
Remove redundant stubs from the MoneyGram and Western Union tests and
ensure that all such stubs result in failure. In particular, the 'offer'
mock is never accessed directly by ReceiptValidator.
Use stricter criteria when deciding which of the taker's accounts (if
any) are valid for a given offer. Specifically, prevent National Bank
accounts from being used to take Same / Specific Bank(s) offers, so the
three payment method types can never being mixed.

This prevents an error on the trading peer when the trade starts, due to
enforcement of equal maker & taker payment method IDs (except for SEPA)
in the Contract payload constructor.

This partially addresses #3602, where the erroneous peer response causes
the taker to be presented with a confusing timeout.
@stejbac stejbac requested a review from ripcurlx as a code owner Nov 24, 2019
Copy link
Member

freimair left a comment

utAck

@freimair freimair mentioned this pull request Nov 26, 2019
@ripcurlx

This comment has been minimized.

Copy link
Member

ripcurlx commented Nov 26, 2019

(Later, it might be worthwhile to make a more substantial change to allow National, Same or Specific
Bank account types to be mixed under certain circumstances when trading, but that could be complicated by the peer running an older version.)

I agree to this patch for now like that, but we should have a look if we are able to solve this as mentioned in a future PR to make it more user friendly.

Copy link
Member

ripcurlx left a comment

utACK

@ripcurlx ripcurlx merged commit e0a92ca into bisq-network:master Nov 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stejbac stejbac mentioned this pull request Dec 13, 2019
@ripcurlx ripcurlx mentioned this pull request Dec 13, 2019
@stejbac stejbac mentioned this pull request Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.