-
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
Integrate PhoneNumberValidator into HalCashForm #3236
Integrate PhoneNumberValidator into HalCashForm #3236
Conversation
Added country selector to HalCashForm, requiring the following changes. 1. A new CountryUtil method getAllHalCashCountries, returns [ES, PL]. 2. HalCashValidator now extends PhoneNumberValidator. 3. HalCashAccountPayload now extends CountryBasedPaymentAccountPayload. 4. HalCashAccount now extends CountryBasedPaymentAccount. 5. Added two new fields to pb.proto. a. HalCashAccountPayload hal_cash_account_payload field to CountryBasedPaymentAccountPayload message. b. accepted_country_codes field to HalCashAccountPayload message. Note: pb.proto changes may break backward compatibility. 6. Reformatted all modified source files except pb.proto. This patch is part of solution to Issue bisq-network#3042.
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 - I think it is not a good idea to add the account creation validation and push it as far to invalidate all existing HalCash accounts for offers created with phone number validated HalCash accounts. Doing it like that it would also force people with correct phone numbers to create a new account and loose their account age. I think it is better for this PR only to add the phone number validation in the account creation form and leave everything else as is.
As it is implemented right now, it is as if we are introducing two new payment methods. HalCash (ES) and HalCash (PL). Also I'm not sure if it is maybe also possible to transfer money with HalCash between ES and PL. |
It is clear you can send international remittances to a halcash networked ATM in other countries. It is not clear halcash users can transfer fiat from one bank account to another, so I think it should be assumed to not be possible, though I don't see how transfers to ATMs in other countries can happen if the funds aren't deposited in a recipient's bank account. |
@ripcurlx, Thanks for the feedback. I'll revert protobuf changes to limit validation to the create payment method use case. Phone validation depends on country. How would you suggest I add a country selector without implying the payment method can be used to transfer fiat between Spain and Poland? |
If it is not possible to send it across borders than the old implementation was false in that regard and what you came up with is the correct implementation. I’ll think about the best case migration scenario. |
According to this EN site, recipients need a cell phone (not email), and access to any HalCash ATM in Spain or Poland. Money can be sent across borders, is not deposited in the recipient's bank account, and HalCash ATM withdrawal limits apply.
According to this HalCash FAQ, there are plans to add US ATM and POS systems to the network. But FYI, this page was published in 2016, which makes me doubt the US roll out is going to happen. |
But isn't that our use case? So someone wants to buy BTC with Halcash and sends the code to the phonenumber of the recipient. The recipient goes to a Halcash ATM receiveds the money and releases the BTC. To make that possible we shouldn't split up Halcash on a country basis, but just validate a general phone number on creation or leave it as it is right now. |
Leaving it as is and closing PR. |
Added country selector to HalCashForm, requiring the following changes.
a. HalCashAccountPayload hal_cash_account_payload field to
CountryBasedPaymentAccountPayload message.
b. accepted_country_codes field to HalCashAccountPayload message.
Note: pb.proto changes may break backward compatibility.
This patch is part of solution to Issue #3042.