-
Notifications
You must be signed in to change notification settings - Fork 101
Add initial Bitrefill widget to staging #3343
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
base: staging-bitrefill
Are you sure you want to change the base?
Add initial Bitrefill widget to staging #3343
Conversation
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.
Very nice LGTM, added some small comments
also @Beerosagos please review if you have a moment.
@@ -21,7 +21,7 @@ export const getExchangeRegionCodes = (): Promise<string[]> => { | |||
return apiGet('exchange/region-codes'); | |||
}; | |||
|
|||
export type TPaymentMethod = 'card' | 'bank-transfer' | 'bancontact' | 'sofort'; | |||
export type TPaymentMethod = 'card' | 'bank-transfer' | 'bancontact' | 'sofort' | 'spend'; |
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.
nit: 'spend' looks a bit wrong here, but I haven't yet checked why it is needed…
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.
Yeah it's a bit ugly. I'm basically abusing the payment method field to pass a custom message (which depends on the provider). Strictly speaking "spend" is the payment method here, i.e. the wallet itself pays.
import { A } from '../anchor/anchor'; | ||
import style from './terms.module.css'; | ||
import { isBitcoinOnly } from '@/routes/account/utils'; | ||
import { localeMapping } from '@/routes/exchange/bitrefill'; |
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.
bitrefill-terms.tsx imports from bitrefill.tsx and bitrefill.tsx imports bitrefill-term.tsx.
Altough it is ok to have circular imports (dependencies) in ES6 modules, we try to not have them
Please move the localeMapping to it's own module (bitrefill-locales.ts ?) so that you can import into both bitrefill.tsx and bitrefill-terms.tsx
} catch (e) { | ||
return null; | ||
} | ||
}; |
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.
nit: we have same (similar?) function in btcdirect, this could be shared in a new `utils/url.ts or similar.
hl: i18n.resolvedLanguage ? localeMapping[i18n.resolvedLanguage] : 'en', | ||
paymentMethods: account.coinCode ? coinMapping[account.coinCode] : 'bitcoin', | ||
refundAddress: bitrefillInfo.address, | ||
// Option to keep pending payment information longer in session, defaults to 'false' |
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.
could you add a link to their documentation somewhere (in this file or in commit message)?
})}</h2>}/> | ||
) : ( | ||
<Header title={<h2>{title}</h2>}/> | ||
)} |
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.
nit: an alternative could be to use 1 <Header
element and move the condition into title attribute.
a90ae85
to
4440b2a
Compare
Integrates Bitrefill widget into the backend and adds a new "spend" action, as Bitrefill is a new type of exchange next to buy and sell.
Integrates Bitrefill in the frontend. On the previous "Buy & sell" menu, there is now a third selectable pill "Spend", opening the available exchange options (Bitrefill in this case). The widget is requested from bitboxapp.shiftcrypto.io using config parameters from the user/app, handled in the "request-configuration" event. Note that the iframe will still have its source set to "https://embed.bitrefill.com". When the user clicks "Pay" inside the Bitrefill widget to purchase a gift card, the returned address and amount is used to propose a transaction on the connected BitBox. The PaymentMethod component is adjusted to allow custom messages to be shown in the exchange overview, as these will differ in the "Spend" section between providers. Disclaimer and info texts are added technically, but the actual text is still missing in this commit. Bitrefill developer documentation: https://www.bitrefill.com/playground/documentation/url-params
57d1a0b
to
b26e651
Compare
@thisconnect addressed comments and also changed to the same tx note format now (as in #3351) |
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.
very nice, initial Bitrefill pr LGTM
Lets get this merged to staging-bitrefill soon, you've already over-delivered and it looks like almost done and more than just a POC :)
ping @Beerosagos ptal
if (result.success) { | ||
const txNote = t('generic.paymentRequestNote', { | ||
name: 'Bitrefill', | ||
invoiceId, |
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.
invoiceId, | |
orderId: invoiceId, |
The placeholder name is orderId
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.
Updated
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.
LGTM but one small issue: orderId: invoiceId
Uses the same "Bitrefill payment XYZ" format for transaction notes of Bitrefill spends, which other integrations use as well.
c1fe7f1
to
0dd92bc
Compare
I already saw the first mention 😂 I'll have a look asap |
I didn't want to rush you, but wanted to hand it over to you. 😇 |
Adds an initial proof of concept integration of the Bitrefill widget.
Still missing: