Crypto payments #284
Crypto payments #284
Conversation
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…est to cf6bad7 (#281) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
| "tweetnacl": "1.0.3", | ||
| "typescript": "5.6.2", | ||
| "vite": "5.4.6" | ||
| }, | ||
| "devDependencies": { | ||
| "@sveltejs/vite-plugin-svelte": "3.1.2", | ||
| "@tsconfig/svelte": "5.0.4" | ||
| "@tsconfig/svelte": "5.0.4", | ||
| "msw": "2.3.1", |
There was a problem hiding this comment.
These were already previously audited - no concerns with the additional dependencies.
|
|
||
| const typedVariants = variants ?? ([] as Array<StrongVariant>); | ||
|
|
||
| // TODO: (maybe) return error if could not find all products in DB |
There was a problem hiding this comment.
Is it worth creating an issue for this TODO and referencing it in the comment?
| const encryptShippingData = async ( | ||
| recipientInformation: App.Recipient | ||
| ): Promise<EncryptedShippingAddress> => { | ||
| const encryptionKey = generateKey(); |
There was a problem hiding this comment.
no concerns with this usage of cryptography here. We're relying on tweetnacl secretbox underneath for this
| displayName: getCustomShippingName(rate) | ||
| })) as Array<ShippingRate>; | ||
| }; | ||
|
|
There was a problem hiding this comment.
nit - looks like GH doesn't like how I'm indenting this
| # The reason we encrypt this data is because we don't want to retain the shipping data longer than | |
| # necessary so the easiest way to do this is to pass it through by the service providers such that once | |
| # payment is complete we can decrypt it and get rid of it and the decryption key while still being able | |
| # to pass it to printful after the payment is completed. |
Helpful comment to explain what's going on in better detail here since it's a relatively key architectural decision that we made here
| }; | ||
| }); | ||
|
|
||
| // TODO: need to figure out how to let users select shipping option? |
There was a problem hiding this comment.
this one seems like it would be good to open a follow up issue for too
There was a problem hiding this comment.
This on is on Radom to implement. I've put in a request for them to match Stripe's functionality for this.
There was a problem hiding this comment.
Gotcha, sounds like nothing to change to our site then just to their page? Would we need to send the additional shipping options to them too?
There was a problem hiding this comment.
Also, btw this is a nit - non-blocker
|
|
||
| export const PROVIDER_NAME = 'radom'; | ||
|
|
||
| export async function radomApi<T = any>(resourcePath: string, options?: RequestInit): Promise<T> { |
There was a problem hiding this comment.
Can you add a comment here saying this code should only be called by backend code?
I want to avoid us accidentally using this in front end code and leaking the API key
There was a problem hiding this comment.
Definitely. Will also look into leveraging $lib/server: https://svelte.dev/docs/kit/server-only-modules#Your-modules
| [key in Network]?: Record<string, string> | ||
| } | ||
|
|
||
| const TestnetTokens: Tokens = { |
There was a problem hiding this comment.
I'm sure this is done somewhere, but I've not come across it yet. Do we check the environment we're in to decide which of these arrays get used somewhere? Just want to make sure we aren't accidentally letting people pay with testnet tokens in prod (or prod in staging/local)
There was a problem hiding this comment.
| @@ -1,4 +1,4 @@ | |||
| /** @type {import("@sveltejs/kit").ParamMatcher} */ | |||
| export function match(param) { | |||
| return /^cs_[a-zA-Z0-9_-]+$/.test(param); | |||
| return /^(?:cs_)?[a-zA-Z0-9_-]+$/.test(param); | |||
There was a problem hiding this comment.
since this appears to be a regex matcher, it probably makes sense to length limit here to 36 characters to avoid a ReDoS issue
There was a problem hiding this comment.
#284 (comment)
and #284 (comment) would be good to get fixed before we merge, but aren't blockers because they're not direct security vulnerabilities. Looking through the rest are nit's so going to approve
ALERT: this PR is still very much a WIP. Once out of draft mode, it will be submitted for security/privacy review.
Resolves #216
This PR introduces a second payment provider, Radom, in order to accept crypto payments.