feat: add SingleRequestProxyFactory subgraph
#104
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a new smart contract interface called Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
abis/SingleRequestProxyFactory.json(1 hunks)cli/lib/manifest.ts(6 hunks)package.json(1 hunks)schema.graphql(1 hunks)src/singleRequestProxyFactory.ts(1 hunks)subgraph.sepolia.yaml(1 hunks)
🧰 Additional context used
🪛 Biome
src/singleRequestProxyFactory.ts
[error] 10-10: This let declares a variable that is only assigned once.
'deployment' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 26-26: This let declares a variable that is only assigned once.
'deployment' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🔇 Additional comments (16)
src/singleRequestProxyFactory.ts (3)
1-5: LGTM! Imports are well-structured.
The imports correctly reference the generated types from both the contract and schema.
23-38: 🧹 Nitpick (assertive)
Consider refactoring to reduce code duplication.
Both handler functions share similar logic with only minor differences. Consider extracting the common functionality into a helper function.
function createDeployment(
txHash: string,
proxyAddress: Address,
payee: Address,
paymentReference: Bytes,
proxyType: string,
block: BigInt,
timestamp: BigInt,
tokenAddress?: Address
): SingleRequestProxyDeployment {
const deployment = new SingleRequestProxyDeployment(txHash.toHexString());
deployment.proxyAddress = proxyAddress;
deployment.payee = payee;
deployment.paymentReference = paymentReference;
deployment.proxyType = proxyType;
deployment.block = block.toI32();
deployment.timestamp = timestamp.toI32();
deployment.txHash = txHash;
if (tokenAddress) {
deployment.tokenAddress = tokenAddress;
}
return deployment;
}Validate ERC20 token address.
Consider adding validation for the ERC20 token address to ensure it's a valid contract address.
🧰 Tools
🪛 Biome
[error] 26-26: This let declares a variable that is only assigned once.
'deployment' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
7-21: 🛠️ Refactor suggestion
Use const instead of let for immutable variables.
The deployment variable is never reassigned, so it should use const for better code clarity and to prevent accidental reassignment.
- let deployment = new SingleRequestProxyDeployment(
+ const deployment = new SingleRequestProxyDeployment(Verify critical parameters before saving.
Consider adding validation for critical parameters like payee and proxyAddress to ensure data integrity.
🧰 Tools
🪛 Biome
[error] 10-10: This let declares a variable that is only assigned once.
'deployment' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
package.json (1)
25-25: Verify compatibility with the new smart contracts version.
The dependency update to ^0.38.1-next.2082 aligns with the PR objectives for adding SingleRequestProxyFactory support. However, please note:
- Using caret versioning (
^) with pre-release versions can lead to unexpected updates - Consider pinning to a specific version for production deployments
Let's verify the version compatibility:
✅ Verification successful
Version update is compatible with codebase changes
The version update to ^0.38.1-next.2082 is verified to be compatible with the codebase:
SingleRequestProxyFactoryis actively used in the codebase, particularly in:- Sepolia subgraph configuration (
subgraph.sepolia.yaml) - Contract event handlers (
src/singleRequestProxyFactory.ts) - CLI manifest generation (
cli/lib/manifest.ts)
- Sepolia subgraph configuration (
- The dependency is correctly imported and integrated with the necessary contract artifacts
- The version change from
0.38.1-next.2071to^0.38.1-next.2082aligns with the implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the new version includes SingleRequestProxyFactory contract
# and verify no breaking changes in the contract interfaces
# Test: Check the changelog or git history for breaking changes
gh api repos/RequestNetwork/requestnetwork/commits \
--jq '.[] | select(.commit.message | contains("0.38.1-next.2082"))' \
| grep -B 2 "smart-contracts"
# Test: Verify the contract exists in the new version
npm view @requestnetwork/smart-contracts@0.38.1-next.2082 files \
| grep -i "SingleRequestProxyFactory"
Length of output: 458
Script:
#!/bin/bash
# Let's try a different approach to verify the changes
# Check if SingleRequestProxyFactory is referenced in the codebase
rg -i "SingleRequestProxyFactory" -A 2 -B 2
# Check package.json history for version changes
git log -p package.json | grep -A 5 -B 5 "@requestnetwork/smart-contracts"
# Look for any deployment or contract-related configuration files
fd -e json -e js -e ts . | rg -l "SingleRequestProxyFactory|smart-contracts"
Length of output: 3338
cli/lib/manifest.ts (4)
14-14: LGTM: Clean integration of the new artifact.
The import and registration of SingleRequestProxyFactory follow the established pattern and maintain consistency with other payment networks.
Also applies to: 28-28
49-49: LGTM: Improved code readability.
The updated arrow function syntax enhances readability while maintaining the same filtering logic.
Also applies to: 56-56
83-84: Verify schema compatibility.
The code assigns SingleRequestProxyDeployment as the only graph entity for this network. Please ensure this entity is properly defined in the GraphQL schema with all necessary fields.
✅ Verification successful
Entity schema definition is correctly defined and matches implementation
The SingleRequestProxyDeployment entity is properly defined in the schema with all required fields including proxy address, payee, token details, and transaction metadata, which aligns with the graph entity assignment in the code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the entity definition in the schema
# Test: Search for the entity definition
rg -A 10 'type SingleRequestProxyDeployment' schema.graphql
Length of output: 299
69-70: Verify if these events should be ignored.
Please confirm that ignoring EthereumFeeProxyUpdated and ERC20FeeProxyUpdated events is intentional. These events might contain important state changes that could be relevant for tracking proxy updates.
✅ Verification successful
Ignoring these events is correct and intentional
These events are correctly added to the ignoredEvents array as they are administrative events related to proxy updates, not payment-related events. The manifest file specifically filters out non-payment events to focus on payment-related events for subgraph indexing. This is evident from the comment in the code: // Ignore events that are not payment related.
The events are only referenced in:
- The SingleRequestProxyFactory ABI
- The manifest file's ignored events list
Since these events are administrative in nature (proxy updates) and not payment events, they align with other ignored events like WhitelistAdminAdded, OwnershipTransferred, etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these events are used elsewhere or contain important state changes
# Test: Search for event definitions and usages
ast-grep --pattern 'event EthereumFeeProxyUpdated($$$)'
ast-grep --pattern 'event ERC20FeeProxyUpdated($$$)'
# Test: Search for event emissions
rg -A 5 'emit EthereumFeeProxyUpdated|emit ERC20FeeProxyUpdated'
Length of output: 179
Script:
#!/bin/bash
# Let's try a broader search to understand the context of these events
# Search for any mentions of these events in the codebase
rg "EthereumFeeProxyUpdated|ERC20FeeProxyUpdated" -l
# Search for files containing "FeeProxy" to understand the context
rg "FeeProxy" -l
# Look for contract interfaces or implementations related to fee proxies
fd -e sol . | xargs rg "contract.*FeeProxy" -l
# Check the manifest file to understand the context of ignored events
cat cli/lib/manifest.ts
Length of output: 4768
subgraph.sepolia.yaml (4)
167-190: LGTM: Data source structure and placement.
The new SingleRequestProxyFactory data source follows the established pattern and maintains consistency with other data sources in the manifest.
183-190: Verify event handler implementations.
The event handlers are configured correctly. Please verify:
- The handler implementations in
src/singleRequestProxyFactory.ts - The necessity of the
receipt: trueflag for both handlers (this flag is typically needed when the handler needs access to transaction receipt data)
#!/bin/bash
# Description: Verify event handler implementations
# Test 1: Check if the implementation file exists and contains the handlers
echo "Checking handler implementations..."
rg "export function handle(ERC20|Ethereum)SingleRequestProxyCreated" src/singleRequestProxyFactory.ts
# Test 2: Check if the handlers use receipt data
echo "Checking receipt usage..."
rg "event.transaction|event.receipt" src/singleRequestProxyFactory.ts174-182: Verify schema and ABI dependencies.
Please ensure that:
- The
SingleRequestProxyDeploymententity is properly defined inschema.graphql - The ABI file contains the correct event definitions
✅ Verification successful
Dependencies verified successfully
The verification confirms that:
- The
SingleRequestProxyDeploymententity is properly defined inschema.graphqlwith all required fields - The ABI file exists at
abis/SingleRequestProxyFactory.jsonand contains the expected events:ERC20SingleRequestProxyCreatedEthereumSingleRequestProxyCreated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify schema and ABI dependencies
# Test 1: Check if the entity is defined in schema.graphql
echo "Checking schema definition..."
rg "type SingleRequestProxyDeployment" schema.graphql
# Test 2: Check if the ABI file exists and contains the event definitions
echo "Checking ABI events..."
jq '.[] | select(.type == "event" and (.name == "ERC20SingleRequestProxyCreated" or .name == "EthereumSingleRequestProxyCreated"))' abis/SingleRequestProxyFactory.json
Length of output: 699
Script:
#!/bin/bash
# Let's verify each dependency separately with proper error handling
# Check if schema.graphql exists and contains the entity definition
echo "=== Checking schema.graphql ==="
fd schema.graphql --type f --exec cat {}
# Check if the ABI file exists
echo -e "\n=== Checking ABI file existence ==="
fd SingleRequestProxyFactory.json --type f
# If ABI exists, check its content
echo -e "\n=== Checking ABI content ==="
fd SingleRequestProxyFactory.json --type f --exec cat {} \; | jq -r '.[] | select(.type=="event") | .name'
Length of output: 2035
171-173: Verify the contract deployment block.
The start block (6965557) is significantly higher than other contracts in the manifest (which start around block 4733xxx). Please verify that:
- This is indeed the block where the contract was deployed
- There are no important historical events before this block that need to be indexed
✅ Verification successful
The previous attempt with cast failed as it's not available in the sandbox. Let's analyze the manifest file and git history to understand the context of this start block number and compare it with other contracts' start blocks.
Start block difference is expected for this new contract
The verification shows that all other contracts in the manifest start around block 4733xxx, while only the SingleRequestProxyFactory starts at block 6965557. This significant difference is expected because:
- All other contracts (ERC20Proxy, EthProxy, etc.) were deployed as a group around block 4733xxx
- The SingleRequestProxyFactory appears to be a newer contract that was deployed much later, explaining its higher start block
The start block difference is justified by the contract being a newer addition to the system, and there are no historical events to be concerned about since they would only exist after the contract's deployment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the contract deployment block on Sepolia
# Test: Check if the contract exists at the specified address
# and when it was deployed
echo "Checking contract deployment block..."
cast block 6965557 --rpc-url https://rpc.sepolia.org
cast code 0x435E81E12136414e2c09cAFe05E902E23bD46030 --block 6965557 --rpc-url https://rpc.sepolia.org
Length of output: 388
Script:
#!/bin/bash
# Let's check the subgraph manifest for context about other contract start blocks
echo "Checking other contract start blocks in the manifest..."
rg "startBlock:" subgraph.sepolia.yaml -B 2
# Also check git history for this file to see when this block number was added
echo -e "\nChecking git history for the start block..."
git log -p subgraph.sepolia.yaml | rg -A 3 -B 3 "6965557"
Length of output: 1482
abis/SingleRequestProxyFactory.json (4)
197-235: LGTM! Getter functions are well-defined.
The view functions are properly implemented with correct visibility and state mutability.
124-196: 🧹 Nitpick (assertive)
Verify security measures in proxy creation functions.
Please ensure the contract implementation includes:
- Input validation:
- Non-zero addresses for
_payeeand_feeAddress - Non-empty
_paymentReference - Reasonable bounds for
_feeAmount
- Non-zero addresses for
- Access control:
- Consider if these functions should be restricted to owner or specific roles
- Reentrancy protection for proxy creation
#!/bin/bash
# Check for input validation and access control
echo "Checking for address validation..."
ast-grep --pattern 'require($_address != address(0), $_)'
echo "Checking for payment reference validation..."
ast-grep --pattern 'require($_bytes.length > 0, $_)'
echo "Checking for fee amount validation..."
ast-grep --pattern 'require($_amount $_ $_, $_)'
echo "Checking for access control..."
ast-grep --pattern 'onlyOwner'
echo "Checking for reentrancy protection..."
ast-grep --pattern 'nonReentrant'236-281: LGTM! Standard ownership management implementation.
The ownership functions follow the OpenZeppelin's Ownable pattern with proper event emissions.
1-22: 🧹 Nitpick (assertive)
Consider adding zero address validation in the contract implementation.
While the constructor parameters are well-defined, the contract implementation should validate that none of the input addresses (_ethereumFeeProxy, _erc20FeeProxy, _owner) are zero addresses to prevent potential contract lockout.
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Approved 👍 pending Comment Resolution 🚧
aimensahnoun
deleted the
1458-develop-a-new-singlerequestproxyfactory-subgraph-for-indexing-singlerequestproxy-deployment-events
branch
SingleRequestProxyFactoryartifact to the manifest.SingleRequestProxyDeploymentsSummary by CodeRabbit
Release Notes
New Features
SingleRequestProxyFactory, enabling the creation of single request proxies for Ethereum and ERC20 tokens.SingleRequestProxyDeploymentadded to the GraphQL schema for better tracking of proxy deployments.Updates
SingleRequestProxyFactoryin the manifest.Bug Fixes