chore(payment-processor): load hinkal lib asyncronously #1573
Conversation
WalkthroughThe changes primarily modify the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client Code
participant P as Payment Module
participant E as EmporiumOp Module
C->>P: Call prepareErc20ProxyPaymentFromHinkalShieldedAddress(...)
P->>E: Await dynamic import of emporiumOp
E-->>P: Return emporiumOp module
P-->>C: Return Promise resolving to IPreparedPrivateTransaction
C->>P: Call prepareErc20FeeProxyPaymentFromHinkalShieldedAddress(...)
P->>E: Await dynamic import of emporiumOp
E-->>P: Return emporiumOp module
P-->>C: Return Promise resolving to IPreparedPrivateTransaction
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alexandre-abrioux
changed the title
github-actions
bot
requested review from
aimensahnoun,
leoslr,
MantisClone and
yomarion
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts (2)
131-134: Update function documentation to reflect async nature.The functions have been correctly made async to support dynamic imports. Consider updating the JSDoc comments to indicate that these functions now return Promises.
/** * Prepare the transaction to privately pay a request through the ERC20 proxy contract, can be used with a Multisig contract. * @param request request to pay * @param amount optionally, the amount to pay. Defaults to remaining amount of the request. + * @returns Promise<IPreparedPrivateTransaction> */Also applies to: 173-177
148-148: Optimize dynamic imports to avoid duplication.The dynamic import of
emporiumOpis duplicated across both functions. Consider extracting it to a shared utility function to improve code maintainability and reduce duplicate code.+async function getEmporiumOp() { + const { emporiumOp } = await import('@hinkal/common'); + return emporiumOp; +} export async function prepareErc20ProxyPaymentFromHinkalShieldedAddress( request: ClientTypes.IRequestData, amount?: BigNumberish, ): Promise<IPreparedPrivateTransaction> { // ... - const { emporiumOp } = await import('@hinkal/common'); + const emporiumOp = await getEmporiumOp(); // ... } export async function prepareErc20FeeProxyPaymentFromHinkalShieldedAddress( request: ClientTypes.IRequestData, amount?: BigNumberish, feeAmountOverride?: BigNumberish, ): Promise<IPreparedPrivateTransaction> { // ... - const { emporiumOp } = await import('@hinkal/common'); + const emporiumOp = await getEmporiumOp(); // ... }Also applies to: 193-193
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts (2)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts:80-99
Timestamp: 2024-12-09T18:59:04.613Z
Learning: In the `RequestNetwork` codebase, payment processor functions such as `payErc20ProxyRequestFromHinkalShieldedAddress` in `packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts` should not include explicit request validation or try/catch blocks, and should rely on the underlying components (like `hinkalObject`) to handle error reporting.
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:0-0
Timestamp: 2024-12-04T05:05:19.610Z
Learning: The function `createRequestForHinkal` in `erc-20-private-payment-hinkal.test.ts` is intended for testing purposes only and should remain in the test directory.
🔇 Additional comments (2)
packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts (2)
1-1: LGTM! Import optimization improves bundle size.The separation of type imports from value imports enables better tree-shaking and async loading capabilities.
Also applies to: 22-22
88-89:✅ Verification successful
Verify all callers handle async prepare functions.
The changes correctly await the prepare functions. However, we should verify that all other callers of these functions in the codebase are updated to handle the async nature.
Also applies to: 115-116
🏁 Script executed:
#!/bin/bash # Search for all usages of the prepare functions to ensure they're properly awaited rg "prepareErc20(Fee)?ProxyPaymentFromHinkalShieldedAddress" -A 2 -B 2Length of output: 2351
Async Callers Verified – Code Update Approved
The changes in
packages/payment-processor/src/payment/erc-20-private-payment-hinkal.tscorrectly useawaitfor bothprepareErc20ProxyPaymentFromHinkalShieldedAddress(lines 88-89) andprepareErc20FeeProxyPaymentFromHinkalShieldedAddress(lines 115-116). The grep results confirm that all current usages within the file properly await the async prepare functions.Please ensure that any future callers also correctly handle the async returns.
There was a problem hiding this comment.
Looks good to me 🙆
As a result of these changes, the hinkal functions for preparing transactions are now asynchronous. I consider this to be a non-breaking change because I assume most users of this code would use the pay... functions rather than the prepare... functions.
Thank you also for cleaning up the import ordering to make it alphabetical.
|
I'm sad to see that one of Hinkal's sub-dependencies is incompatible with a |
|
I haven't tested the Hinkal integration, but it might be fine even without allowing |
|
Hi @alexandre-abrioux and @yomarion, Another dependency in @hinkal/common is snarkjs, which relies on wasm-unsafe-eval. Snarkjs is widely used in zero-knowledge proof applications like Railgun and Penumbra, primarily because it enables in-browser proving. The WASM files used by snarkjs are generated by CIRCOM (https://docs.circom.io/), a toolkit for zk-circuits. Using WASM for client-side proving offers significant performance improvements over JavaScript. Since we are in the process of integrating with Request Finance, do you think it might be possible to adjust the content policy to permit wasm-unsafe-eval? |
|
@nkoreli Thank you for all the details 🙏 At the moment, Request Finance does not require Wasm scripts, so we do not allow it. We can of course reassess during implementation (but let's discuss it internally as this is a security topic). |
Context
The Request Network library is relatively heavy to load from a front-end point of view, more especially the
payment-processorpackage. Here are some stats for a front-end that includes packages needed for payment, and built with Vite.js:> ls -lh build/assets/@requestnetwork/ total 7,3M 308K currency-BI28kHzu.js 128K data-format-CF2Ej1ak.js 1,2M payment-detection-B7AW9tt1.js 5,7M payment-processor-COb8hmRF.js 1 smart-contracts-l0sNRNKZ.js 1 types-l0sNRNKZ.jsChanges
In this PR, I introduce a quick optimization that would reduce the
payment-processorload time and resolve a CSP warning for non-Hinkal users.@hinkal/commonrepresents a large amount of the final code for thepayment-processorpackage after transpilation. It could be loaded asynchronously to lighten frontend bundles. The picture below is a visualization of the space taken by each sub-dependency of thepayment-processorpackage, according to /~https://github.com/btd/rollup-plugin-visualizer.@hinkal/commondepends onlibsodium(also quite heavy, see below) which runs WASM code in the browser. Loading this lib is incompatible with aContent-Security-Policythat does not allowscript-src 'wasm-unsafe-eval'. For builders that do not use the Hinkal integration, it does not make sense to relax the CSP header to silence such a warning. Loading Hinkal only on-demand would not trigger such warnings for non-Hinkal users. Related: Chrome requires unsafe-eval to run webassembly code jedisct1/libsodium.js#196Summary by CodeRabbit