feat: Adding private wallet functionality with the integration tests #1482
Conversation
WalkthroughThe pull request introduces several changes to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (1).circleci/config.yml (1)
The workflow configuration follows best practices:
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 (
|
nkoreli
changed the title
github-actions
bot
requested review from
aimensahnoun,
alexandre-abrioux,
kevindavee and
MantisClone
nkoreli
changed the title
nkoreli
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (7)
packages/payment-processor/package.json(1 hunks)packages/payment-processor/src/payment/erc20-fee-proxy.ts(4 hunks)packages/payment-processor/src/payment/prepare-hinkal.ts(1 hunks)packages/payment-processor/src/payment/prepared-transaction.ts(1 hunks)packages/usage-examples/package.json(2 hunks)packages/usage-examples/src/hinkal/createRequestForHinkal.ts(1 hunks)packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/payment-processor/src/payment/erc20-fee-proxy.ts
[error] 174-174: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (8)
packages/payment-processor/src/payment/prepared-transaction.ts (1)
12-12: Verify Hinkal operation types.
Since we're integrating with Hinkal middleware, we should verify if there are specific types available in the @hinkal/common package for the ops array.
✅ Verification successful
Based on the evidence gathered, I can now provide a final response:
The ops array uses Hinkal's operation format correctly
The code is already properly integrated with Hinkal's types. We can see that:
- The file imports
emporiumOpfrom '@hinkal/common' - The
opsarray is populated using theemporiumOp()function from Hinkal - The operations are used with the
RelayerTransactiontype, also imported from '@hinkal/common'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Hinkal provides specific types for operations
# Test 1: Look for type definitions in Hinkal package
rg -t ts "type.*Operation|interface.*Operation" -g "*.d.ts"
# Test 2: Look for operation-related constants or enums
rg -t ts "enum.*Operation|const.*Operation" -g "*.ts"
Length of output: 415
Script:
#!/bin/bash
# Let's check the actual usage and context of ops in the codebase
# Look for ops usage in payment processor
rg -t ts "ops" -A 3 -B 3 packages/payment-processor/src/payment/
# Check imports in the file
rg -t ts "^import.*from.*@hinkal" packages/payment-processor/src/payment/prepared-transaction.ts
# Look for any operation-related types in the entire codebase
rg -t ts "type.*Op|interface.*Op|enum.*Op"
Length of output: 5766
Script:
#!/bin/bash
# Let's check the emporiumOp function since it's used to create the ops
rg -t ts "emporiumOp" -A 5 -B 5
# Also check if there are any type definitions for the operation string format
rg -t ts "function.*emporiumOp"
Length of output: 2304
packages/payment-processor/src/payment/prepare-hinkal.ts (3)
13-15:
Address ethers.js version compatibility issue.
The comment indicates a type discrepancy due to different ethers.js versions. This could lead to runtime issues.
Let's check for version conflicts:
#!/bin/bash
# Description: Check for potential ethers.js version conflicts
# Expected: Consistent ethers.js versions across dependencies
# Check package.json for ethers versions
rg -l 'ethers":\s*"[^"]+' 15-16: Clarify the purpose of undefined parameter in initProviderAdapter.
The undefined parameter in initProviderAdapter needs explanation. Consider adding a comment explaining why it's undefined or provide the correct value.
Let's check the expected usage:
#!/bin/bash
# Description: Check Hinkal documentation or usage patterns
# Expected: Find examples of initProviderAdapter usage
# Search for other initProviderAdapter calls
rg 'initProviderAdapter' 1-3: Verify @hinkal/common package version and security.
Since we're introducing a new dependency for handling private transactions, we should ensure we're using a stable, audited version.
Let's check the package details:
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts (2)
1-5: LGTM! Imports and configuration are well structured.
The necessary dependencies are properly imported and environment configuration is appropriately handled.
11-25: Verify the integration with Hinkal middleware.
The code interacts with Hinkal middleware for private transactions. Let's verify the integration points.
✅ Verification successful
Let me check the implementation details of the Hinkal integration to verify it properly.
Let me check one final aspect - the ERC20 fee proxy implementation to verify the complete integration flow.
Based on the comprehensive review of the implementation, I can now provide the final response:
The Hinkal integration is properly implemented and verified.
The integration is well-structured with:
- Correct initialization of Hinkal middleware in
prepare-hinkal.ts - Proper implementation of private transaction handling in
erc20-fee-proxy.ts - Complete request creation flow in
createRequestForHinkal.ts - Appropriate dependency on
@hinkal/commonversion 0.0.167
The code follows the recommended patterns for private transactions:
- Uses proper preprocessing and initialization
- Implements merkle tree reset
- Correctly handles signer integration
- Properly manages private wallet actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Hinkal integration points and dependencies
echo "Checking Hinkal integration points..."
# Check if createRequestForHinkal is properly implemented
echo "Checking createRequestForHinkal implementation..."
rg -l "createRequestForHinkal" --type ts
# Check for Hinkal dependencies
echo "Checking Hinkal dependencies..."
rg -l "@hinkal" --type json
# Check for other examples or tests using Hinkal
echo "Checking for other Hinkal usage..."
rg -l "Hinkal" --type ts
Length of output: 860
Script:
#!/bin/bash
# Check the implementation of createRequestForHinkal
echo "Content of createRequestForHinkal.ts:"
cat packages/usage-examples/src/hinkal/createRequestForHinkal.ts
echo -e "\nContent of prepare-hinkal.ts:"
cat packages/payment-processor/src/payment/prepare-hinkal.ts
echo -e "\nChecking Hinkal dependency version:"
cat packages/payment-processor/package.json | grep -A 5 -B 5 "@hinkal"
Length of output: 3959
Script:
#!/bin/bash
# Check ERC20 fee proxy implementation for Hinkal integration
echo "Content of erc20-fee-proxy.ts:"
cat packages/payment-processor/src/payment/erc20-fee-proxy.ts
# Check for any tests
echo -e "\nChecking for related tests:"
rg -l "prepareHinkal" --type test
Length of output: 8326
packages/payment-processor/package.json (1)
42-42: Verify the stability and compatibility of @hinkal/common.
The dependency is pinned to version 0.0.167, which indicates a pre-1.0 release. Given that this package handles critical private transaction functionality:
- Ensure this version has been thoroughly tested with ethers 5.5.1
- Verify there are no known security vulnerabilities
- Consider waiting for a stable 1.0+ release before production use
✅ Verification successful
Let me gather more information about this dependency and its usage in the codebase.
Let me analyze the usage of @hinkal/common to verify its integration with ethers.
Based on the code analysis, I can now provide a final response about the compatibility and stability concerns.
@hinkal/common dependency is properly integrated with ethers
The implementation shows proper integration between @hinkal/common and ethers:
- The package exports
exportEthersProviderand integrates with ethers'Signer - It's used in a type-safe manner with proper ethers imports
- The integration pattern follows the ethers 5.5.1 API which is the version used across the codebase
While the version (0.0.167) is pre-1.0, the implementation demonstrates:
- Clean integration with ethers through proper typing
- Consistent usage pattern across payment processor files
- Clear separation of concerns between private transaction handling and core functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and compatibility issues
# Test: Check for any security advisories for @hinkal/common
npm audit @hinkal/[email protected]
# Test: Check the peer dependencies to verify compatibility
npm info @hinkal/[email protected] peerDependencies
Length of output: 451
Script:
#!/bin/bash
# Check if and how @hinkal/common is being used in the codebase
rg "@hinkal/common" -A 3 -B 3
# Check package.json files for ethers version requirements
fd "package.json" --exec grep -l "ethers" {} \; | xargs cat
# Look for any usage patterns of ethers with @hinkal/common
ast-grep --pattern 'import $_ from "@hinkal/common"'
# Check for any documentation about @hinkal/common integration
fd "README.md" --exec grep -l "hinkal" {} \; | xargs cat
Length of output: 30623
Script:
#!/bin/bash
# Check the implementation files that use @hinkal/common to verify ethers integration
rg -A 10 -B 10 "from '@hinkal/common'" packages/payment-processor/src/
# Check if there are any direct ethers imports alongside @hinkal/common
rg "import.*ethers" packages/payment-processor/src/payment/prepare-hinkal.ts
rg "import.*ethers" packages/payment-processor/src/payment/erc20-fee-proxy.ts
# Look for any configuration or setup files related to Hinkal
fd "hinkal|private" -t f packages/payment-processor/src/
Length of output: 3465
packages/usage-examples/package.json (1)
28-29: LGTM! Verify the test file exists.
The new script addition aligns with the PR objectives for providing a working example.
✅ Verification successful
Script exists and is properly implemented
The test file exists at the correct location and contains a well-structured implementation for testing private transactions using Hinkal on Optimism. The file properly imports required dependencies, uses environment variables for private key handling, and implements the expected functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the test file exists
if [ ! -f "packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts" ]; then
echo "Error: Test file not found"
exit 1
fi
Length of output: 88
Script:
#!/bin/bash
# Let's use fd to search for the test file with a more flexible approach
fd "testPayErc20FeeProxyRequestHinkal.ts" packages/usage-examples/
# Also check the content if file exists to verify it's a proper test file
rg -l "testPayErc20FeeProxyRequestHinkal" packages/usage-examples/
Length of output: 242
Script:
#!/bin/bash
# Let's verify the content of the test file to ensure it's properly implemented
cat packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts
Length of output: 1129
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
packages/payment-processor/src/payment/erc20-fee-proxy.ts(4 hunks)packages/payment-processor/src/payment/prepare-hinkal.ts(1 hunks)packages/payment-processor/src/payment/prepared-transaction.ts(1 hunks)packages/usage-examples/src/hinkal/createRequestForHinkal.ts(1 hunks)packages/usage-examples/src/hinkal/hinkalRequestData.ts(1 hunks)packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/payment-processor/src/payment/prepare-hinkal.ts (1)
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/src/payment/prepare-hinkal.ts:14-17
Timestamp: 2024-11-04T12:17:38.858Z
Learning: In `packages/payment-processor/src/payment/prepare-hinkal.ts`, we prefer not to add try-catch blocks in the `prepareHinkal` function for initialization failures, as rethrowing errors with slightly different messages provides little benefit and may make it harder to detect the exact type of error thrown.
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts (3)
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:19-27
Timestamp: 2024-11-04T12:18:13.888Z
Learning: In the file `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts`, when using `payPrivateErc20FeeProxyRequest`, the returned `tx` is a `RelayerTransaction`, so calling `.wait()` on it is unnecessary.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:19-27
Timestamp: 2024-11-04T12:18:13.888Z
Learning: In usage examples where errors are displayed directly to the user running the script (e.g., `testPayErc20FeeProxyRequestHinkal.ts`), adding a `try/catch` block may not provide significant benefits.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:0-0
Timestamp: 2024-11-04T12:17:58.700Z
Learning: Private key validation is handled by ethers.js, so explicit validation in our code is not necessary.
🪛 Biome
packages/payment-processor/src/payment/erc20-fee-proxy.ts
[error] 173-173: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (12)
packages/usage-examples/src/hinkal/hinkalRequestData.ts (2)
1-3: LGTM! Imports are appropriate and necessary.
7-8: Verify network configuration usage across the codebase.
The network configuration looks correct for the Optimism deployment mentioned in the PR objectives.
✅ Verification successful
Let me check the actual usage of these network configurations to ensure consistency.
Network configuration is correctly used and consistent
The verification shows that:
- The
currentNetworkvalue 'optimism' is properly imported and used increateRequestForHinkal.tsfor both network and payment network configurations - The network configuration aligns with the supported networks in the payment detection client
- The native currency configuration also supports the Optimism network
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent network configuration usage
# Expected: All references should use 'optimism' as the network
# Search for network configurations
rg -l "currentNetwork|ChainName.*optimism"
# Search for potential hardcoded network values
rg -l "network.*optimism"
Length of output: 421
Script:
#!/bin/bash
# Check the content of files that reference currentNetwork
rg -A 2 "currentNetwork" packages/usage-examples/src/hinkal/createRequestForHinkal.ts packages/usage-examples/src/hinkal/hinkalRequestData.ts
# Check network configurations in currency and payment detection
rg -A 2 "network.*optimism" packages/currency/src/native.ts packages/payment-detection/src/thegraph/client.ts
# Check for any other chain names or network configurations
ast-grep --pattern 'ChainName = $_'
Length of output: 1804
packages/payment-processor/src/payment/prepare-hinkal.ts (2)
23-24: Consider persisting Merkle tree state.
The resetMerkle call suggests the Merkle tree state is being reset on each initialization. This might not be optimal for maintaining transaction history.
Consider:
- Documenting why the Merkle tree needs to be reset
- Evaluating if state persistence would be beneficial
- Adding logic to determine when reset is actually needed
19-19: Document ethers.js version requirements.
The comment indicates a type discrepancy due to different ethers.js versions. This could lead to runtime issues.
Let's check the ethers.js version requirements:
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts (4)
1-5: LGTM! Imports and configuration are properly set up.
The necessary dependencies are imported, and the dotenv configuration is correctly initialized.
7-12: LGTM! Documentation is comprehensive and includes security considerations.
The comments effectively cover setup instructions, prerequisites, and security considerations.
19-21: LGTM! Provider and signer setup follows best practices.
The implementation correctly uses ethers.js for provider and signer creation.
2-3: Verify Hinkal integration and function signatures.
Let's verify the correct usage of Hinkal-related functions and their integration.
Also applies to: 24-25
✅ Verification successful
Let me try a different approach to verify the Hinkal integration.
Based on the results, I can now generate the final response:
✓ Hinkal integration and function signatures are correctly implemented
The verification shows:
createRequestForHinkalis properly implemented inpackages/usage-examples/src/hinkal/createRequestForHinkal.tswith correct parameters (wallet and privateKey)payPrivateErc20FeeProxyRequestis correctly imported from@requestnetwork/payment-processorand implemented inpackages/payment-processor/src/payment/erc20-fee-proxy.ts- The integration flow in
testPayErc20FeeProxyRequestHinkal.tsfollows the expected pattern:- Creates request data using
createRequestForHinkal - Processes payment using
payPrivateErc20FeeProxyRequest
- Creates request data using
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of Hinkal-related functions
# Check the imported function signatures
echo "Checking function signatures..."
ast-grep --pattern 'export function payPrivateErc20FeeProxyRequest($params) { $$$ }'
# Check the implementation of createRequestForHinkal
echo "Checking createRequestForHinkal implementation..."
ast-grep --pattern 'export function createRequestForHinkal($params) { $$$ }'
Length of output: 347
Script:
#!/bin/bash
# Search for files containing Hinkal-related code
echo "Files containing Hinkal-related code:"
rg -l "Hinkal" --type ts
# Check the content of createRequestForHinkal
echo -e "\nContent of files with createRequestForHinkal:"
rg "createRequestForHinkal" -A 5 --type ts
# Check usage of payPrivateErc20FeeProxyRequest
echo -e "\nUsage of payPrivateErc20FeeProxyRequest:"
rg "payPrivateErc20FeeProxyRequest" -A 5 --type ts
# Check package exports
echo -e "\nChecking package exports:"
fd "index.ts|index.js" --type f --exec rg "export.*Hinkal|export.*payPrivateErc20FeeProxyRequest" {}
Length of output: 4585
packages/usage-examples/src/hinkal/createRequestForHinkal.ts (1)
39-78:
Add privacy-specific parameters and error handling
The request parameters and creation process need to be enhanced for private transactions using Hinkal.
Consider these enhancements:
const requestCreateParameters: Types.ICreateRequestParameters = {
requestInfo: {
currency: {
type: currentCurrenyType,
value: currencyAddress,
network: currentNetwork,
},
+ // Add privacy configuration
+ privacy: {
+ enabled: true,
+ stealth: {
+ address: await generateStealthAddress(payee),
+ },
+ zkProof: {
+ enabled: true,
+ // Add ZK-proof parameters
+ }
+ },
// ... other parameters
},
// ... other configurations
};
-// Step 4: create & send request
-const request = await requestClient.createRequest(requestCreateParameters);
-const requestData = await request.waitForConfirmation();
-return requestData;
+// Step 4: Validate privacy requirements and create request
+try {
+ // Validate privacy requirements
+ if (!await validatePrivacyRequirements(requestCreateParameters)) {
+ throw new Error('Privacy requirements not met');
+ }
+
+ const request = await requestClient.createRequest(requestCreateParameters);
+
+ // Wait for both confirmation and privacy proof generation
+ const [requestData, privacyProof] = await Promise.all([
+ request.waitForConfirmation(),
+ generatePrivacyProof(request)
+ ]);
+
+ // Verify privacy proof
+ if (!await verifyPrivacyProof(privacyProof)) {
+ throw new Error('Privacy proof verification failed');
+ }
+
+ return {
+ ...requestData,
+ privacyProof
+ };
+} catch (error) {
+ throw new Error(`Failed to create private request: ${error.message}`);
+}Likely invalid or redundant comment.
packages/payment-processor/src/payment/erc20-fee-proxy.ts (3)
Line range hint 1-28: LGTM: Import changes are well-structured
The new imports are properly organized and necessary for implementing the private payment functionality.
70-75: Verify the negative amount usage in Hinkal's actionPrivateWallet
The function passes a negative amount (-amountToPay) to Hinkal's actionPrivateWallet. While this might be intentional for the Hinkal protocol, it's important to verify this behavior.
#!/bin/bash
# Search for other usages of Hinkal's actionPrivateWallet to verify the amount sign convention
rg -A 5 "actionPrivateWallet"
# Search for Hinkal documentation or tests regarding amount sign
rg -A 5 "amount.*negative|negative.*amount" 174-175: Verify proxy contract address against trusted source
For security-critical operations, it's important to verify the proxy contract address against a trusted source.
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts
Outdated
Show resolved
Hide resolved
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
packages/payment-processor/src/payment/prepare-hinkal.ts(1 hunks)packages/payment-processor/src/payment/prepared-transaction.ts(1 hunks)packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts(1 hunks)
🧰 Additional context used
📓 Learnings (3)
packages/payment-processor/src/payment/prepare-hinkal.ts (2)
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/src/payment/prepare-hinkal.ts:14-27
Timestamp: 2024-11-04T14:31:29.780Z
Learning: When reviewing PRs, if ethers v6 support is not relevant to the PR, avoid suggesting implementation of ethers.js version compatibility strategies.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/src/payment/prepare-hinkal.ts:14-17
Timestamp: 2024-11-04T12:17:38.858Z
Learning: In `packages/payment-processor/src/payment/prepare-hinkal.ts`, we prefer not to add try-catch blocks in the `prepareHinkal` function for initialization failures, as rethrowing errors with slightly different messages provides little benefit and may make it harder to detect the exact type of error thrown.
packages/payment-processor/src/payment/prepared-transaction.ts (1)
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/src/payment/prepared-transaction.ts:15-16
Timestamp: 2024-11-04T14:31:29.664Z
Learning: In `packages/payment-processor/src/payment/prepared-transaction.ts`, for the `IPreparedPrivateTransaction` interface, use `string[]` for the `ops` field since Hinkal's relevant functions accept arrays of `string` as inputs.
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts (4)
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:14-18
Timestamp: 2024-11-04T14:32:19.189Z
Learning: RPC URL validation is not required in the TypeScript file `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts` in the `usage-examples` package, as per the user's guidance.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:19-27
Timestamp: 2024-11-04T12:18:13.888Z
Learning: In the file `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts`, when using `payPrivateErc20FeeProxyRequest`, the returned `tx` is a `RelayerTransaction`, so calling `.wait()` on it is unnecessary.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:19-27
Timestamp: 2024-11-04T12:18:13.888Z
Learning: In usage examples where errors are displayed directly to the user running the script (e.g., `testPayErc20FeeProxyRequestHinkal.ts`), adding a `try/catch` block may not provide significant benefits.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:0-0
Timestamp: 2024-11-04T12:17:58.700Z
Learning: Private key validation is handled by ethers.js, so explicit validation in our code is not necessary.
🔇 Additional comments (5)
packages/payment-processor/src/payment/prepare-hinkal.ts (1)
1-3: LGTM: Imports are clean and focused.
The imports are minimal and properly scoped to the requirements of the module.
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts (4)
1-6: LGTM! Imports and configuration are well structured.
The necessary dependencies are imported and dotenv is configured appropriately at the start of the file.
19-22: LGTM! Provider and signer setup is correct.
The provider and wallet creation follows ethers.js best practices.
1-32: Verify integration with payment processor and Hinkal.
Let's verify the integration points and dependencies.
✅ Verification successful
Integration with payment processor and Hinkal is properly implemented
The verification shows:
- Correct integration with
@requestnetwork/payment-processorusingpayPrivateErc20FeeProxyRequest - Proper Hinkal initialization in the payment processor
- Clean dependency chain from test file through payment processor to Hinkal
- No circular dependencies detected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify integration with payment processor and Hinkal
# Check payment processor integration
echo "Checking payment processor integration..."
rg -p "payPrivateErc20FeeProxyRequest" --type ts
# Check Hinkal integration
echo "Checking Hinkal integration..."
rg -p "import.*@hinkal/common" --type ts
# Check for potential circular dependencies
echo "Checking for circular dependencies..."
rg -p "createRequestForHinkal.*testPayErc20FeeProxyRequestHinkal" --type ts
Length of output: 1036
Script:
#!/bin/bash
# Let's verify the createRequestForHinkal implementation and its dependencies
# Check createRequestForHinkal implementation
echo "Checking createRequestForHinkal implementation..."
rg -p "export.*createRequestForHinkal" -A 10 --type ts
# Check Hinkal initialization and usage
echo "Checking Hinkal initialization..."
ast-grep --pattern 'new Hinkal($$$)'
# Check payment processor ERC20 fee proxy implementation
echo "Checking payment processor implementation..."
rg -p "function payPrivateErc20FeeProxyRequest" -A 10 --type ts
Length of output: 1562
14-18: 🧹 Nitpick (assertive)
Consider adding top-level await for better error handling.
The void IIFE pattern works but using top-level await would provide cleaner error handling and exit codes.
-void (async () => {
+// Using top-level await for cleaner error handling
+try {
const { PAYER_PRIVATE_KEY, OPTIMISM_RPC_URL } = process.env;
if (!PAYER_PRIVATE_KEY) throw new Error('PRIVATE_KEY_MISSING');
const RPC_URL = OPTIMISM_RPC_URL || 'https://mainnet.optimism.io';
-
+} catch (error) {
+ console.error('Error:', error);
+ process.exit(1);
+}Likely invalid or redundant comment.
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts
Outdated
Show resolved
Hide resolved
|
@nkoreli @giorgi-kiknavelidze Am I correct that this PR supersedes #1477 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/payment-processor/src/payment/erc20-fee-proxy.ts (2)
62-62: Add error handling for Hinkal preparationConsider adding try-catch block to handle potential errors from
prepareEthersHinkal.- const hinkal = await prepareEthersHinkal(getSigner(signerOrProvider)); + let hinkal; + try { + hinkal = await prepareEthersHinkal(getSigner(signerOrProvider)); + } catch (error) { + throw new Error(`Failed to prepare Hinkal: ${error.message}`); + }
173-173: Consider explicit network validationThe non-null assertion operator (
!) onnetworkcould lead to runtime errors. Consider explicit validation.- EvmChains.assertChainSupported(network!); + if (!network) { + throw new Error('Network information is required for private transactions'); + } + EvmChains.assertChainSupported(network);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/payment-processor/package.json(1 hunks)packages/payment-processor/src/payment/erc20-fee-proxy.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/payment-processor/package.json
🧰 Additional context used
📓 Learnings (1)
packages/payment-processor/src/payment/erc20-fee-proxy.ts (2)
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/src/payment/erc20-fee-proxy.ts:186-186
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In the `preparePrivateErc20FeeProxyPaymentTransaction` function in `packages/payment-processor/src/payment/erc20-fee-proxy.ts`, we should not make an allowance call on the token contract, as it is not relevant to our use case.
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1487
File: packages/payment-processor/test/payment/single-request-proxy.test.ts:237-246
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/payment-processor/test/payment/single-request-proxy.test.ts`, when asserting the `feeProxyUsed` in emitted events from `SingleRequestProxyFactory`, retrieve the `erc20FeeProxy` (or `ethereumFeeProxy`) public variable from the `SingleRequestProxyFactory` contract instead of using `wallet.address`.
🔇 Additional comments (4)
packages/payment-processor/src/payment/erc20-fee-proxy.ts (4)
1-18: LGTM: Import statements are properly organized
The new imports are correctly structured and necessary for the private payment functionality.
28-29: LGTM: Interface imports are properly updated
The addition of IPreparedPrivateTransaction is consistent with the new private payment functionality.
185-195: Remove token approval operation
Based on the learnings from previous reviews, the token approval operation is not relevant for this use case and should be removed.
const ops = [
- emporiumOp(tokenContract, 'approve', [proxyContract.address, amountToPay]),
emporiumOp(proxyContract, 'transferFromWithReferenceAndFee', [
tokenAddress,
paymentAddress,
amountToPay,
`0x${paymentReference}`,
feeToPay,
feeAddress,
]),
];70-75: Verify the negation of amount in private wallet action
The amount is being negated (-amountToPay) when passed to actionPrivateWallet. Please verify this is the intended behavior for private transactions.
✅ Verification successful
The negation of amount in private wallet action is correct
The negation of amountToPay in actionPrivateWallet is the intended behavior. This is because:
- The function
preparePrivateErc20FeeProxyPaymentTransactionfirst approves the token spending and then transfers the tokens through the proxy contract, which means the amount should be deducted from the user's wallet. - The negative sign indicates a debit operation from the private wallet, consistent with the payment flow where tokens are being sent out.
- This matches the regular (non-private) payment flow where the amount is deducted from the sender's balance, as seen in the test file where the balance check confirms:
BigNumber.from(balanceErc20After).eq(BigNumber.from(balanceErc20Before).sub(102))
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of actionPrivateWallet to verify amount handling
ast-grep --pattern 'actionPrivateWallet($_,$_,$_,$_)'
Length of output: 55
Script:
#!/bin/bash
# Search for files containing 'actionPrivateWallet'
rg -l "actionPrivateWallet"
# Then search for the actual usage with context
rg "actionPrivateWallet" -A 5 -B 5
Length of output: 939
Script:
#!/bin/bash
# Let's look at the broader context of this file to understand the payment flow
cat packages/payment-processor/src/payment/erc20-fee-proxy.ts
# Also search for any tests that might validate this behavior
rg -l "erc20-fee-proxy" --glob "**/*test*"
Length of output: 8805
Script:
#!/bin/bash
# Let's check the test file to see how this behavior is validated
cat packages/payment-processor/test/payment/erc20-fee-proxy.test.ts
Length of output: 8215
There was a problem hiding this comment.
The code looks good, but I wish the PR description included more explanation about how to use the feature and what sort of privacy can be achieved.
- Does it work for all three privacy configurations: Private to Private, Private to Public, Public to Private?
- What are the steps a payer would take in the Private to Public configuration?
Is it possible to add integration tests showing the different privacy configurations - perhaps with assertions showing that the sender and receiver addresses used by the ERC20FeeProxy are different from the addresses that deposit to Hinkal?
|
From @giorgi-kiknavelidze:
|
|
If you end up adding an integration test that uses external APIs, we can configure it to run nightly instead of on every PR. Going further, if the integration test requires real funds, then we can reduce it to weekly or monthly. My intention is to have a happy path test that can prevent regressions in the future. |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (8)
packages/usage-examples/src/hinkal/hinkalRequestData.ts (3)
5-6: Add documentation for the USDC contract address and amount.While hardcoding the USDC address is acceptable, please add a comment specifying this is for the Base network. Also, consider documenting why such a small test amount (0.0001 USDC) was chosen.
-export const currencyAddress = '0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913'; // USDC -export const currencyAmount = ethers.utils.parseUnits('0.0001', 6).toBigInt(); +// USDC contract address on Base network +export const currencyAddress = '0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913'; +// Small test amount (0.0001 USDC) to minimize costs during testing +export const currencyAmount = ethers.utils.parseUnits('0.0001', 6).toBigInt();
7-9: Fix typo in variable name 'currentCurrenyType'.There's a spelling error in the variable name.
-export const currentCurrenyType = Types.RequestLogic.CURRENCY.ERC20; +export const currentCurrencyType = Types.RequestLogic.CURRENCY.ERC20;
10-15: Improve payment details configuration.Several improvements needed:
- The due date should follow ISO 8601 format
- Add more descriptive comments about the payee address
- Document why the fee is set to '0'
-export const payee = '0xA4faFa5523F63EE58aE7b56ad8EB5a344A19F266'; // some random address -export const fee = '0'; -export const contentData = { - reason: 'Hinkal Test', - dueDate: '2025.06.16', -}; +// Test payee address - replace with actual recipient in production +export const payee = '0xA4faFa5523F63EE58aE7b56ad8EB5a344A19F266'; + +// Transaction fee - set to 0 for testing purposes +export const fee = '0'; + +export const contentData = { + reason: 'Hinkal Test', + dueDate: '2025-06-16T00:00:00Z', // ISO 8601 format +};packages/payment-processor/src/index.ts (1)
6-6: LGTM! Consider adding documentation.The export follows the established naming convention and is logically placed among other ERC20-related exports. This aligns well with the PR's objective of adding private wallet functionality using Hinkal.
Consider adding a comment above this export to briefly document the Hinkal integration and its purpose, similar to:
+// Export Hinkal private payment functionality for ERC20 tokens export * from './payment/erc-20-private-payment-hinkal';packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (1)
109-149: Enhance test coverage with additional scenariosWhile the current tests verify the basic privacy aspects, consider adding these scenarios:
- Error cases (invalid private key, network issues)
- Edge cases (zero amount, max token amount)
- Different fee configurations
- Transaction failure scenarios
Example additional test:
it('should handle network errors gracefully', async () => { const invalidProvider = new ethers.providers.JsonRpcProvider('http://invalid-url'); const invalidPayer = new ethers.Wallet(PAYER_PRIVATE_KEY, invalidProvider); await expect(createRequestForHinkal( invalidPayer, PAYER_PRIVATE_KEY, Types.Extension.PAYMENT_NETWORK_ID.ERC20_PROXY_CONTRACT )).rejects.toThrow(); });packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts (3)
25-25: Typo in comment: 'intergration' should be 'integration'There's a typo in the comment on line 25: 'intergration' should be 'integration'.
34-54: Consider refactoring to reduce code duplicationThe functions
payPrivateErc20ProxyRequestFromHinkalandpayPrivateErc20FeeProxyRequestFromHinkalhave similar structures and logic. Refactoring common code into a shared helper function could enhance maintainability and reduce redundancy.Also applies to: 61-80
28-33: Incorrect JSDoc comment: Function does not handle feesThe comment for
payPrivateErc20ProxyRequestFromHinkalstates that the function processes a transaction "privately with fees", but this function does not handle fees. This might lead to confusion.Update the comment to accurately reflect the function's purpose, indicating that it processes transactions without fees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
packages/payment-processor/package.json(1 hunks)packages/payment-processor/src/index.ts(1 hunks)packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts(1 hunks)packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts(1 hunks)packages/usage-examples/package.json(1 hunks)packages/usage-examples/src/hinkal/createRequestForHinkal.ts(1 hunks)packages/usage-examples/src/hinkal/hinkalRequestData.ts(1 hunks)packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/payment-processor/package.json
- packages/usage-examples/package.json
- packages/usage-examples/src/hinkal/createRequestForHinkal.ts
🧰 Additional context used
📓 Learnings (2)
packages/usage-examples/src/hinkal/hinkalRequestData.ts (1)
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/hinkalRequestData.ts:5-6
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/usage-examples/src/hinkal/hinkalRequestData.ts`, it's acceptable to hardcode the USDC contract address without creating a mapping of USDC assets on specific networks.
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts (5)
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:23-32
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts`, the team has decided not to implement a delay or graceful shutdown handling after `process.exit(0)`, as they consider it unnecessary.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:14-18
Timestamp: 2024-11-12T14:52:33.204Z
Learning: RPC URL validation is not required in the TypeScript file `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts` in the `usage-examples` package, as per the user's guidance.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:19-27
Timestamp: 2024-11-12T14:52:33.204Z
Learning: In the file `packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts`, when using `payPrivateErc20FeeProxyRequest`, the returned `tx` is a `RelayerTransaction`, so calling `.wait()` on it is unnecessary.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:19-27
Timestamp: 2024-11-12T14:52:38.580Z
Learning: In usage examples where errors are displayed directly to the user running the script (e.g., `testPayErc20FeeProxyRequestHinkal.ts`), adding a `try/catch` block may not provide significant benefits.
Learnt from: giorgi-kiknavelidze
PR: RequestNetwork/requestNetwork#1482
File: packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts:0-0
Timestamp: 2024-11-12T14:52:38.580Z
Learning: Private key validation is handled by ethers.js, so explicit validation in our code is not necessary.
🪛 Biome (1.9.4)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts
[error] 12-13: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 13-14: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 14-15: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 15-16: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 16-17: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 17-18: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 18-19: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 35-107: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🪛 Gitleaks (8.21.2)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts
22-22: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts
11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
packages/usage-examples/src/hinkal/hinkalRequestData.ts (1)
1-3: LGTM: Imports are appropriate and well-structured.
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts (2)
14-17: LGTM!
The provider and signer setup follows ethers.js best practices.
18-27: LGTM!
The transaction flow is well-implemented with appropriate logging and follows the team's established patterns for handling RelayerTransactions.
packages/payment-processor/src/index.ts (1)
6-6: Verify the referenced module exists
The new export statement references './payment/erc-20-private-payment-hinkal'. Let's ensure this module is present in the codebase.
✅ Verification successful
Export statement references a valid module
The referenced module erc-20-private-payment-hinkal.ts exists at the correct location in the codebase: packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the referenced module exists
fd --type f "erc-20-private-payment-hinkal.ts" packages/payment-processor/src/payment/
Length of output: 158
packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts (1)
48-48: Verify passing negative amountToPay in actionPrivateWallet
The amounts parameter passed to hinkal.actionPrivateWallet includes -amountToPay. Ensure that passing a negative value here is correct and aligns with the expected API usage.
Check the actionPrivateWallet API documentation to confirm that negative amounts are intended and will produce the correct behavior.
Also applies to: 74-74
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts
Outdated
Show resolved
Hide resolved
packages/usage-examples/src/hinkal/testPayErc20FeeProxyRequestHinkal.ts
Outdated
Show resolved
Hide resolved
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts
Outdated
Show resolved
Hide resolved
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts
Outdated
Show resolved
Hide resolved
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts
Outdated
Show resolved
Hide resolved
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts
Outdated
Show resolved
Hide resolved
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts
Outdated
Show resolved
Hide resolved
packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts
Outdated
Show resolved
Hide resolved
packages/payment-processor/src/payment/erc-20-private-payment-hinkal.ts
Outdated
Show resolved
Hide resolved
… key" This reverts commit 1cf6f4a.
github-actions
bot
requested review from
alexandre-abrioux,
kevindavee and
MantisClone
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (4)
27-35: Document required environment variables.Add a comment block listing all required environment variables and their expected formats:
/** * Required environment variables: * - HINKAL_TEST_PAYER_PRIVATE_KEY: Private key for the payer's wallet * - HINKAL_TEST_PAYER_ADDRESS: Public address of the payer * - HINKAL_TEST_PAYEE_ADDRESS: Public address of the payee * - HINKAL_TEST_PAYEE_PRIVATE_KEY: Private key for the payee's wallet * - HINKAL_TEST_PAYEE_SHIELDED_ADDRESS: Shielded address of the payee */
39-39: Document the reason for long timeout.Add a comment explaining why such a long timeout is necessary:
// Set a long timeout (16.67 minutes) as tests interact with real blockchain and require multiple transactions jest.setTimeout(1000000);
139-141: Improve error message in getTokenShieldedBalance.The error message could be more descriptive to help with debugging.
- throw new Error(`No balance found for token ${tokenAddress} at address ${address}`); + throw new Error( + `No balance found for token ${tokenAddress} at address ${address}. ` + + `Available balances: ${JSON.stringify(balances.map(b => ({ + token: b.token.erc20TokenAddress, + balance: b.balance.toString() + })))}` + );
180-182: Consider adding more specific balance checks.The current balance check only verifies that the balance increased. Consider adding checks for:
- The exact amount transferred
- The fee amount (if applicable)
- expect(BigNumber.from(balanceErc20Before).lt(BigNumber.from(balanceErc20After))); + const expectedIncrease = BigNumber.from(currencyAmount); + const actualIncrease = BigNumber.from(balanceErc20After).sub(BigNumber.from(balanceErc20Before)); + expect(actualIncrease.eq(expectedIncrease)).toBe(true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.circleci/config.yml(3 hunks)packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (5)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:191-192
Timestamp: 2024-12-18T05:27:21.653Z
Learning: Em testes que utilizam uma cadeia privada, não é necessário aguardar confirmações de bloco (p.ex. tx.wait(2)), pois o ambiente interno não se comporta como uma rede pública.
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:176-177
Timestamp: 2024-12-18T05:27:01.757Z
Learning: In private chain tests, waiting for block confirmations is not strictly required.
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
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.
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.
🔇 Additional comments (3)
.circleci/config.yml (1)
270-283: LGTM! Well-structured CI configuration
The changes appropriately:
- Add a new
test-monthlyjob for running Hinkal tests - Configure a 30-minute timeout for long-running tests
- Set up a monthly schedule trigger
Also applies to: 368-382
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (2)
37-37: LGTM: Mainnet RPC URL is intentionally used for real on-chain tests.
The use of mainnet RPC URL is approved as it's intentionally used for real on-chain tests.
162-183: LGTM: Well-structured test with clear objectives.
The test case is well organized with:
- Clear objectives documented
- Privacy verification checks
- Balance verification
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (4)
36-36: Consider reducing the test timeout.The current timeout of 1000000ms (16.67 minutes) per test seems excessive. Consider reducing it to a more reasonable value like 60000ms (1 minute) unless there's a specific reason for such a long timeout.
-jest.setTimeout(1000000); // Set Jest timeout for asynchronous operations (e.g., blockchain calls) +jest.setTimeout(60000); // Set timeout to 1 minute for blockchain operations
27-32: Add documentation for required environment variables.The test file requires environment variables but lacks setup documentation. Consider adding a comment block explaining the required variables and their format.
Add this documentation above the environment variables:
/** * Required environment variables: * HINKAL_TEST_PAYER_PRIVATE_KEY - Private key for the payer's wallet * HINKAL_TEST_PAYEE_PRIVATE_KEY - Private key for the payee's wallet * * Format: 0x-prefixed hex string */
120-125: Standardize wait times across tests.The
waitLittlefunction is called with different durations (7s, 5s, and 1s) in different tests. This inconsistency could lead to flaky tests if the shorter duration is insufficient.Consider using a constant for the wait duration:
+const BALANCE_INDEX_WAIT_TIME = 7; // seconds -const waitLittle = (time = 15): Promise<number> => +const waitLittle = (time = BALANCE_INDEX_WAIT_TIME): Promise<number> =>
162-258: Consider enhancing test coverage with additional assertions.While the current tests cover the basic functionality, consider adding:
- Verification of event emissions
- Error cases (e.g., insufficient balance)
- Edge cases (e.g., zero amount transfers)
Example assertion for event verification:
expect(tx).toHaveEmittedWith('Transfer', { from: tx.from, to: payeeWallet.address, value: currencyAmount });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
packages/payment-processor/package.json(1 hunks)packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/payment-processor/package.json
🧰 Additional context used
📓 Learnings (1)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (5)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:191-192
Timestamp: 2024-12-18T05:27:21.653Z
Learning: Em testes que utilizam uma cadeia privada, não é necessário aguardar confirmações de bloco (p.ex. tx.wait(2)), pois o ambiente interno não se comporta como uma rede pública.
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:176-177
Timestamp: 2024-12-18T05:27:01.757Z
Learning: In private chain tests, waiting for block confirmations is not strictly required.
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
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.
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.
🔇 Additional comments (1)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (1)
148-160: LGTM! Well-structured test setup and cleanup.
The setup properly initializes all required components and the cleanup ensures proper resource management by clearing both Hinkal snapshots and Jest timers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (1)
36-36: Consider documenting the reason for high timeout value.The timeout is set to ~16 minutes which is quite high. While this might be necessary for the tests to pass, it would be helpful to add a comment explaining why such a long timeout is required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (5)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:191-192
Timestamp: 2024-12-18T05:27:21.653Z
Learning: Em testes que utilizam uma cadeia privada, não é necessário aguardar confirmações de bloco (p.ex. tx.wait(2)), pois o ambiente interno não se comporta como uma rede pública.
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:176-177
Timestamp: 2024-12-18T05:27:01.757Z
Learning: In private chain tests, waiting for block confirmations is not strictly required.
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1482
File: packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts:38-38
Timestamp: 2024-12-18T03:53:54.370Z
Learning: In `packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts`, mainnet RPC is intentionally used for real on-chain tests as confirmed by MantisClone.
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.
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.
🔇 Additional comments (2)
packages/payment-processor/test/payment/erc-20-private-payment-hinkal.test.ts (2)
14-34: LGTM! Constants are well-configured for mainnet testing.
The configuration is appropriate for real on-chain tests:
- Sensitive data (private keys) is properly handled using environment variables
- Constants are well documented with clear purposes
- Mainnet RPC URL is intentionally used for real on-chain testing
143-259: Well-structured test suites with comprehensive privacy coverage!
The test implementation is thorough and well-organized:
- Clear test objectives and documentation
- Proper setup and cleanup
- Comprehensive coverage of privacy aspects for both sender and recipient
- Good balance validation logic
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.circleci/config.yml(2 hunks)packages/payment-processor/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/payment-processor/package.json
🔇 Additional comments (1)
.circleci/config.yml (1)
367-381: LGTM! Monthly workflow configuration is well structured.
The workflow configuration follows CircleCI best practices with appropriate scheduling and branch filtering.
Fixes #1520
Hinkal Private Wallet Integration
Hinkal is a middleware and suite of smart contracts on EVM-compatible chains that leverage ZK-proofs and stealth addresses to facilitate compliant and private transactions across major dApps. Users can securely store assets and interact with on-chain protocols while maintaining privacy and compliance.
Hinkal allows three types of private transactions:
This integration enables payers/payees to send/receive payments privately. PR introduces support for Hinkal private payments via ERC20 tokens.
We have introduced three new private payment functions:
The function parameters remain the same as
payErc20FeeProxyRequest. However, forpayErc20ProxyRequestFromHinkalShieldedAddressandpayErc20FeeProxyRequestFromHinkalShieldedAddress, the return object is now aRelayerTransactioninstead ofContractTransaction, since the transaction is executed by third parties (Relayers).To make private to public payments, a payer must call
payErc20ProxyRequestFromHinkalShieldedAddress/payErc20FeeProxyRequestFromHinkalShieldedAddress. A payer with a positive shielded balance can call a private payment function, and his wallet address will not appear in the transaction. Instead, the address of a Relayer will appear on-chain as the origin of the transaction.Note: In private-to-public payments, Relayers act as the on-chain intermediaries for submitting transactions. The payer composes the transaction’s calldata—complete with a zero-knowledge proof of sufficient funds—and then provides it to a Relayer. From there, the Relayer broadcasts the transaction on-chain, enabling the payment to be processed while preserving the payer’s privacy.
To make private to public transactions, a payer must have a positive shielded balance. To deposit into his own shielded balance, the payer should call
sendToHinkalShieldedAddressFromPublicwith recipientInfo left undefined.In public to private transactions, the payer sends funds to the shielded address of the payee in the Hinkal smart contract. To send funds to the shielded address of the payee, one should use
sendToHinkalShieldedAddressFromPublicwith recipientInfo. In this case, the recipient of the funds will be the shielded address of the payee inside the Hinkal smart contract, rather than an EOA of the payee.Integration Tests
We added an integration test with four cases:
The integration test flow is conducted on the Base Network using a test account with $1 in its shielded balance. USDC is used as the transaction asset. Each integration test costs $0.009 (based on gas prices as of 11/28/2024).
For the first two cases:
The integration test flow creates a request object, calls a Hinkal contract, which in turn calls a Request Network contract.
Objectives of this test:
Steps:
payErc20ProxyRequestFromHinkalShieldedAddress/payErc20FeeProxyRequestFromHinkalShieldedAddress.For the third case:
For illustration: we show how payer can send funds to his own shielded address.
The payer needs to do this to be able to send funds from his shielded address.
For the fourth case:
Objectives of this test:
Steps:
sendToHinkalShieldedAddressFromPublic.The first private transaction successfully executed on the Request Network using Hinkal: link
And here's another more recent transaction: link
Considerations
Strongly consider using Hinkal Private Payments with encrypted requests so that the payee identity address and payer identity address used to create the request also remain private.
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests