-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
Blink wallet sending attachment #1293
Conversation
Is there some discussion in https://github.com/GaloyMoney/blink-mobile or elsewhere that I can subscribe to for updates regarding this? I searched for "CORS" but found nothing. And do you have any feedback regarding the wallet documentation or ease of implementation? Was it straight-forward or did you have to read a lot of the existing code to get an idea how to add a wallet (additionally to the documentation)? You are the first contributor who added a wallet so we're very curious how easy it was and if there is anything lacking that we could improve. |
The conversation happened in the galoy dev chat, but there are these two related prs: Also, it seems it went to production 1 hour ago, so, later today i will rebase this pr and do a final test and then it should be ready to go 👍
It was pretty straight-forward actually, the only doubt i had was if i had to put these variable in index.js or client.js stacker.news/wallets/blink/index.js Lines 3 to 5 in b7b4e46
the documentation seems to imply that index.js should export only the specified properties , but i ended up putting them there anyway since they are variables that server.js could use at some point, not sure if this respect the design |
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.
It was pretty straight-forward actually
Cool!
the documentation seems to imply that index.js should export only the specified properties , but i ended up putting them there anyway since they are variables that server.js could use at some point, not sure if this respect the design
That's fine, you're good.
Regarding the code: I unfortunately couldn't test the payment code since Blink apparently isn't available in the US.1 But looking at the API docs, it looks good. Left some suggestions regarding other stuff that I'd like to see resolved before merging.
Thank you for the PR!
Footnotes
-
I used an American phone number during signup and later it said my IP address is not allowed to signup. ↩
The current master branch has something that affects this pr, it seems something broke validation for optional fields in commit ae73b0c . I don't know if this is an issue with the master branch or if i need to add something to the field to make it pass some validation. |
Formik validation must see 'currency' as undefined and apply the default but the validation before save sees an empty string.
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.
The current master branch has something that affects this pr, it seems something broke validation for optional fields in commit ae73b0c .
The 'currency' field in the blink connector is defaulted to "BTC" by client.js, if unspecified, however since ae73b0c if the field is left empty in the attach screen, the wallet does not attach and redirects back to settings/wallets without error.
I've tried to rebase this pr on the commit before and it works as expected there.
You're right, something changed. We now run validation twice since ae73b0c: once via Formik props and once just before saving.
This is the case since the validation Formik runs just makes sure receiving OR sending works while the validation before save makes sure the configuration we're about to save is valid (spending config is saved on the client, while receiving config is saved on the server).
The save no longer works since currency
is set to an empty string at that point which means the default doesn't apply so the oneOf
check fails. It fails silently since it's assumed at least one save will work if Formik validation worked.
We can workaround this by returning the default using transform
if currency
is the empty string:
diff --git a/lib/validate.js b/lib/validate.js
index 776cacdd..39ba6e63 100644
--- a/lib/validate.js
+++ b/lib/validate.js
@@ -717,8 +717,7 @@ export const blinkSchema = object({
.required('required')
.matches(/^blink_[A-Za-z0-9]+$/, { message: 'must match pattern blink_A-Za-z0-9' }),
currency: string()
- .transform(value => value.toUpperCase())
- .default('BTC')
+ .transform(value => value ? value.toUpperCase() : 'BTC')
.oneOf(['USD', 'BTC'], 'must be BTC or USD')
})
I went ahead and pushed these changes (and other); I hope you don't mind.
Description
Adds blink sending attachment. Solves #1142 .
Screenshots
Additional Context
This attachment currently works only with the blink's staging environment due to an issue in cors configuration, the pr should stay on hold until the fix is pushed to production.Checklist
Are your changes backwards compatible? Please answer below:
Yes
Did you QA this? Could we deploy this straight to production? Please answer below:
Yes
For frontend changes: Tested on mobile? Please answer below:
Yes
Did you introduce any new environment variables? If so, call them out explicitly here:
No