feat: add params to IR factory #198
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.tsOops! Something went wrong! :( ESLint: 8.57.1 TypeError: prettier.resolveConfig.sync is not a function 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant changes to the Changes
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 3
🧹 Outside diff range and nitpick comments (8)
packages/zevm-app-contracts/hardhat.config.ts (1)
55-61: Consider enhancing mainnet configuration security and reliabilityWhile the configuration is functional, there are several production-grade improvements to consider:
- Using a public RPC endpoint (
blockpi.network) for mainnet could lead to rate limiting or reliability issues- The
gasMultiplierof 3 might be excessive and lead to higher transaction costs- The private keys are directly used from environment variables without additional validation
Consider these improvements:
- Use multiple RPC endpoints with fallback mechanism
- Implement a more conservative gas multiplier (1.2-1.5 range)
- Add environment variable validation
zeta_mainnet: { accounts: PRIVATE_KEYS, chainId: 7000, gas: "auto", - gasMultiplier: 3, + gasMultiplier: 1.2, - url: `https://zetachain-evm.blockpi.network/v1/rpc/public`, + url: process.env.ZETA_MAINNET_RPC_URL || "https://zetachain-evm.blockpi.network/v1/rpc/public", },Add at the top of the file:
if (process.env.NODE_ENV === 'production' && !process.env.ZETA_MAINNET_RPC_URL) { throw new Error('ZETA_MAINNET_RPC_URL is required for production deployment'); }packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol (2)
16-16: Event parameter indexing may not be efficient for stringsIndexing the
stringparameternamein theInstantRewardsCreatedevent can be costly due to the way strings are handled in event logs. Since strings are hashed when indexed, it may not provide meaningful filtering capability. Consider using abytes32or avoiding indexing thenameif not essential.
43-52: Align parameter order for clarity and maintainabilityIn the instantiation of
InstantRewardsV2, the parameters are passed in an order that may not align with the constructor's definition or logical grouping. Consider ordering the parameters logically, grouping related parameters together (e.g., all URLs together).Apply this diff to reorder the parameters:
InstantRewardsV2 instantRewards = new InstantRewardsV2( signerAddress, owner(), start, end, name, - promoUrl, avatarUrl, + promoUrl, description );Ensure that this change corresponds to the constructor's parameter order in
InstantRewardsV2.packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts (2)
51-60: Update test setup to handle new constructor parametersThe added parameters in the
InstantRewardsV2deployment require corresponding assertions to validate their correctness. Consider adding tests to verify that these parameters are set as expected after deployment.Add assertions to check the new parameters:
expect(await instantRewards.name()).to.equal("Instant Rewards"); expect(await instantRewards.promoUrl()).to.equal("http://img.com"); expect(await instantRewards.avatarUrl()).to.equal("http://avatar.com"); expect(await instantRewards.description()).to.equal("Description");
Line range hint
334-343: Test description does not match the test logicThe test case is titled "Should be able to withdraw an active IR," but there is no assertion or verification of the withdrawal outcome. Include assertions to confirm that the withdrawal behaves as expected during the active period.
Add assertions to validate the withdrawal:
await expect( instantRewards.withdraw(user.address, amountToWithdraw) ).to.emit(instantRewards, "Withdrawn").withArgs(user.address, amountToWithdraw); const contractBalance = await ethers.provider.getBalance(instantRewards.address); expect(contractBalance).to.equal(amount.sub(amountToWithdraw));packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol (2)
Line range hint
21-38: Consider grouping related constructor parametersThe constructor parameters mix core functionality (addresses, timeframe) with metadata. Consider reordering parameters to group related fields together.
constructor( address signerAddress_, address owner, uint256 start_, uint256 end_, - string memory name_, - string memory promoUrl_, - string memory avatarUrl_, - string memory description_ + // Metadata group + string memory name_, + string memory description_, + string memory promoUrl_, + string memory avatarUrl_ )
Line range hint
61-63: Critical: Restore withdraw validation or document security implicationsThe removal of the active status check in the withdraw function could allow premature withdrawal of funds before the reward period ends. This is a significant security change that could affect user trust.
Additionally, consider adding an event emission for withdrawals to improve transparency.
function withdraw(address wallet, uint256 amount) public override onlyOwner { + if (isActive()) revert InstantRewardStillActive(); super.withdraw(wallet, amount); + emit Withdrawal(wallet, amount); } + event Withdrawal(address indexed wallet, uint256 amount);packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2.ts (1)
Line range hint
1-81: Add test coverage for modified withdraw behaviorThe removal of withdraw validation requires comprehensive test coverage to ensure security. Add test cases for:
- Withdrawal during active period
- Withdrawal after period ends
- Multiple withdrawals
Example test structure:
describe("withdraw", () => { it("Should allow withdrawal after period ends", async () => { // Setup and advance time past end await ethers.provider.send("evm_increaseTime", [end + 1]); await expect(instantRewards.withdraw(wallet.address, amount)) .to.emit(instantRewards, "Withdrawal") .withArgs(wallet.address, amount); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol(4 hunks)packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol(2 hunks)packages/zevm-app-contracts/data/addresses.json(2 hunks)packages/zevm-app-contracts/hardhat.config.ts(2 hunks)packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts(2 hunks)packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2-compatibility.ts(3 hunks)packages/zevm-app-contracts/test/instant-rewards/instant-rewards-v2.ts(3 hunks)
🔇 Additional comments (7)
packages/zevm-app-contracts/hardhat.config.ts (1)
43-44: LGTM: BlockScout explorer URLs updated correctly
The updated BlockScout URLs for the testnet are correct and align with the current ZetaChain testnet infrastructure.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol (2)
4-4: Reverting to Ownable may reduce security during ownership transfers
The change from Ownable2Step to Ownable alters the ownership handover process from a two-step procedure to a single-step, which can be less secure. Consider whether this change is intentional and assess its impact on the contract's security and administrative controls.
54-54: Ensure consistency in event emission parameters
The InstantRewardsCreated event now includes the name parameter. Ensure that all emitted parameters are accurate and consistent with the event definition, especially if additional parameters are added in the future.
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts (2)
10-10: Confirm the ownership address is correct
The owner address has been updated. Verify that 0xD7E8bD37db625a4856E056D2617C9d140dB99182 is the intended owner, and that it aligns with your deployment and administration plans.
24-24: Update the address label to match the deployed contract
The address is being saved under the key "InstantRewardsFactory", which reflects the deployment of the InstantRewardsFactory contract. Ensure that this change is intentional and consistent across your deployment scripts and configuration files.
packages/zevm-app-contracts/data/addresses.json (2)
15-16: Ensure the new InstantRewardsFactory address is correctly integrated
The addition of "InstantRewardsFactory" with address "0x02F054A5BeeB2653d1c2403cBe9B262356fdD5E4" requires that all components referencing this contract are updated accordingly. Verify that any services, scripts, or front-end applications consuming this address are adjusted to prevent inconsistencies.
26-26: Confirm the updated InstantRewards address for mainnet
The InstantRewards address on mainnet has changed. Ensure that this update is reflected across all environments and that any dependencies or integrations are using the correct address.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsV2.sol
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
packages/zevm-app-contracts/scripts/explorer.helpers.ts (1)
7-18: Consider reducing code duplication in verification logicThe current implementation duplicates the
runcall with nearly identical parameters. Consider consolidating the logic to improve maintainability.- if (contract) { - await run("verify:verify", { - address: contractAddress, - constructorArguments, - contract, - }); - } else { - await run("verify:verify", { - address: contractAddress, - constructorArguments, - }); - } + await run("verify:verify", { + address: contractAddress, + constructorArguments, + ...(contract && { contract }), + });packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts (1)
23-24: Extract magic numbers into named constantsThe timestamp calculations use magic numbers for time calculations. Consider extracting these into named constants for better readability and maintainability.
+const WEEK_IN_SECONDS = 60 * 60 * 24 * 7; +const CAMPAIGN_START_DELAY = WEEK_IN_SECONDS; +const CAMPAIGN_DURATION = WEEK_IN_SECONDS * 4; - const start = timestamp + 60 * 60 * 24 * 7; // 1 week from now - const end = start + 60 * 60 * 24 * 7 * 4; // 4 weeks from start + const start = timestamp + CAMPAIGN_START_DELAY; + const end = start + CAMPAIGN_DURATION;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol(1 hunks)packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol(4 hunks)packages/zevm-app-contracts/data/addresses.json(2 hunks)packages/zevm-app-contracts/scripts/explorer.helpers.ts(1 hunks)packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/zevm-app-contracts/data/addresses.json
🔇 Additional comments (3)
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol (2)
30-33: Add input validation for new URL and description parameters
The new parameters promoUrl, avatarUrl, and description are accepted without validation.
43-53: 🛠️ Refactor suggestion
Consider emitting event before external contract creation
The InstantRewardsCreated event is emitted after creating the new contract. Consider emitting it before to follow the checks-effects-interactions pattern and potentially save gas.
+ emit InstantRewardsCreated(address(instantRewards), owner(), name);
InstantRewardsV2 instantRewards = new InstantRewardsV2(
signerAddress,
owner(),
start,
end,
name,
promoUrl,
avatarUrl,
description
);
- emit InstantRewardsCreated(address(instantRewards), owner(), name);
return address(instantRewards);Likely invalid or redundant comment.
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol (1)
Line range hint 36-93: Implementation Review: Strong Security Foundations
The implementation demonstrates several security best practices:
- Robust signature verification using EIP712
- Proper reentrancy protection with
nonReentrantmodifier - Comprehensive event emissions for tracking
- Clear error handling with custom errors
- Appropriate access controls
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewardsFactory.sol
Show resolved
Hide resolved
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts
Outdated
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Show resolved
Hide resolved
packages/zevm-app-contracts/contracts/instant-rewards/InstantRewards.sol
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts (3)
23-24: Extract time calculations to configurationTime calculations are hardcoded. Consider moving these to configuration for better maintainability:
+const TIME_CONFIG = { + REWARD_START_DELAY: 7 * 24 * 60 * 60, // 1 week + REWARD_DURATION: 4 * 7 * 24 * 60 * 60, // 4 weeks +}; + - const start = timestamp + 60 * 60 * 24 * 7; // 1 week from now - const end = start + 60 * 60 * 24 * 7 * 4; // 4 weeks from start + const start = timestamp + TIME_CONFIG.REWARD_START_DELAY; + const end = start + TIME_CONFIG.REWARD_DURATION;
64-66: Add error handling for contract deploymentAdd proper error handling and deployment confirmation:
- const instantRewards = await InstantRewardsFactory.deploy(owner); - await instantRewards.deployed(); + const instantRewards = await InstantRewardsFactory.deploy(owner) + .catch((error) => { + console.error('Contract deployment failed:', error); + throw error; + }); + + console.log('Waiting for deployment confirmation...'); + await instantRewards.deployed() + .catch((error) => { + console.error('Contract deployment confirmation failed:', error); + throw error; + });
Line range hint
77-86: Enhance error handling with custom error typesConsider implementing custom error types for better error handling:
+class DeploymentError extends Error { + constructor(message: string, public readonly cause?: unknown) { + super(message); + this.name = 'DeploymentError'; + } +} const main = async () => { if (!isProtocolNetworkName(networkName)) throw new Error("Invalid network name"); const instantRewards = await deployInstantRewards(); await deployInstantRewardsSample(instantRewards); }; -main().catch((error) => { - console.error(error); +main().catch((error: unknown) => { + if (error instanceof DeploymentError) { + console.error('Deployment failed:', error.message); + if (error.cause) console.error('Caused by:', error.cause); + } else { + console.error('Unexpected error:', error); + } process.exit(1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/zevm-app-contracts/data/addresses.json(2 hunks)packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/zevm-app-contracts/data/addresses.json
🔇 Additional comments (4)
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts (4)
11-14: Consider moving OWNERS addresses to configuration file
Hardcoded addresses should be moved to a configuration file for better maintainability and to follow separation of concerns.
36-38: Avoid hardcoding gas limit
The gas limit is hardcoded to 25000000. Consider making this configurable or estimating it dynamically.
45-48: Improve type safety in event handling
The code uses TypeScript ignores and unsafe type assertions. Consider using proper type guards and event parsing.
26-34: 🛠️ Refactor suggestion
Move metadata to configuration file
Static metadata like URLs and descriptions should be configuration-driven:
+interface RewardMetadata {
+ name: string;
+ promoUrl: string;
+ avatarUrl: string;
+ description: string;
+}
+
+const REWARD_METADATA: Record<string, RewardMetadata> = {
+ zeta_mainnet: {
+ name: "ZetaChain",
+ promoUrl: "https://zetachain.io",
+ avatarUrl: "https://zetachain.io/logo.png",
+ description: "ZetaChain description"
+ }
+};
+
+const metadata = REWARD_METADATA[networkName];
+if (!metadata) {
+ throw new Error(`No metadata configured for network: ${networkName}`);
+}
+
const params = [
owner,
start,
end,
- "ZetaChain",
- "https://zetachain.io",
- "https://zetachain.io/logo.png",
- "ZetaChain description",
+ metadata.name,
+ metadata.promoUrl,
+ metadata.avatarUrl,
+ metadata.description,
];Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts (4)
11-14: 🛠️ Refactor suggestionMove OWNERS addresses to configuration file
Hardcoded addresses in the source code reduce maintainability and violate separation of concerns.
Create a
config.tsfile to store network-specific configurations:// config.ts export const NETWORK_CONFIG = { zeta_mainnet: { owner: "0xD7E8bD37db625a4856E056D2617C9d140dB99182", // other network-specific config... }, zeta_testnet: { owner: "0x1d24d94520B94B26351f6573de5ef9731c48531A", // other network-specific config... } } as const;
16-17:⚠️ Potential issueRemove @ts-ignore and implement proper type checking
Replace the unsafe type assertion with proper type checking:
-//@ts-ignore -const owner = OWNERS[networkName]; +const owner = OWNERS[networkName as keyof typeof OWNERS]; +if (!owner) { + throw new Error(`No owner configured for network: ${networkName}`); +}
42-44: 🛠️ Refactor suggestionAvoid hardcoded gas limit
Use dynamic gas estimation instead of hardcoded values:
- const tx = await instantRewards.createInstantRewards(...params, { - gasLimit: 25000000, - }); + const gasEstimate = await instantRewards.estimateGas.createInstantRewards(...params); + const tx = await instantRewards.createInstantRewards(...params, { + gasLimit: Math.ceil(gasEstimate * 1.2), // Add 20% buffer + });
51-54:⚠️ Potential issueImprove type safety in event handling
Replace unsafe type assertions with proper type handling:
- if (!event) throw new Error("InstantRewardsCreated event not found"); - //@ts-ignore - const instantRewardsAddress = event.args[0]; - if (!instantRewardsAddress) throw new Error("InstantRewards address not found"); + if (!event || !event.args) throw new Error("InstantRewardsCreated event not found"); + const [instantRewardsAddress, , ] = event.args; + if (!ethers.utils.isAddress(instantRewardsAddress)) { + throw new Error("Invalid InstantRewards address"); + }
🧹 Nitpick comments (2)
packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts (2)
29-30: Extract time constants to configurationHardcoded time durations reduce maintainability and make the code less flexible.
Consider extracting these to configuration:
const TIME_CONFIG = { DEPLOYMENT_DELAY_DAYS: 7, REWARD_DURATION_WEEKS: 4, } as const; const start = timestamp + (TIME_CONFIG.DEPLOYMENT_DELAY_DAYS * 24 * 60 * 60); const end = start + (TIME_CONFIG.REWARD_DURATION_WEEKS * 7 * 24 * 60 * 60);
78-79: Remove commented out codeCommented out code reduces readability and should be removed if no longer needed.
- await verifyContract(instantRewards.address, [owner]); - // await verifyContract("0xAf5693bBC958e442462F411F46421e389c7A8602", [owner]); + await verifyContract(instantRewards.address, [owner]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/zevm-app-contracts/data/addresses.json(2 hunks)packages/zevm-app-contracts/scripts/instant-rewards/deploy-v2.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/zevm-app-contracts/data/addresses.json
andresaiello
deleted the
andy/pd-6916-ir-dedicated-walletsmart-contract
branch
Summary
Summary by CodeRabbit
New Features
InstantRewardsFactorycontract with additional parameters for creating instant rewards.InstantRewardsV2contract, including promotional and avatar URLs, and a description.zeta_testnetand added new settings forzeta_mainnet.Bug Fixes
InstantRewardsV2contract to allow withdrawals even when rewards are active.Chores