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/hardhat.config.tsOops! Something went wrong! :( ESLint: 8.57.1 TypeError: prettier.resolveConfig.sync is not a function 📝 WalkthroughWalkthroughThis pull request introduces significant changes to the 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: 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.
| string memory name, | ||
| string memory promoUrl, | ||
| string memory avatarUrl, | ||
| string memory description |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating new string parameters to prevent empty values
The new parameters promoUrl, avatarUrl, and description are accepted without validation. To ensure data integrity, consider adding checks to validate that these strings are not empty and conform to expected formats.
Apply this diff to add validation:
+ if (bytes(promoUrl).length == 0) revert EmptyPromoUrl();
+ if (bytes(avatarUrl).length == 0) revert EmptyAvatarUrl();
+ if (bytes(description).length == 0) revert EmptyDescription();Ensure to define the new error types accordingly.
Committable suggestion skipped: line range outside the PR's diff.
| string public promoUrl; | ||
| string public avatarUrl; | ||
| string public description; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding length validation for metadata fields
The new public string variables could lead to excessive gas costs during deployment if the strings are too long. Consider adding length validation in the constructor.
+ error MetadataFieldTooLong();
+ uint256 private constant MAX_METADATA_LENGTH = 512; // Adjust as needed
constructor(...) {
+ if (
+ bytes(promoUrl_).length > MAX_METADATA_LENGTH ||
+ bytes(avatarUrl_).length > MAX_METADATA_LENGTH ||
+ bytes(description_).length > MAX_METADATA_LENGTH
+ ) revert MetadataFieldTooLong();
// existing code
}Committable suggestion skipped: line range outside the PR's diff.
| const tx = instantRewardsFactory | ||
| .connect(owner) | ||
| .createInstantRewards(signer.address, start, end, name, "http://img.com", "http://avatar.com", "Description"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for metadata fields
The current tests only verify happy path scenarios with hardcoded values. Consider adding test cases for:
- Empty strings
- Maximum length validation
- Special characters in URLs
- Invalid URL formats
Example test structure:
it("Should revert when metadata exceeds maximum length", async () => {
const longString = "a".repeat(1000);
await expect(
instantRewardsFactory.createInstantRewards(
signer.address,
start,
end,
name,
longString,
"http://avatar.com",
"Description"
)
).to.be.revertedWith("MetadataFieldTooLong");
});Also applies to: 51-59, 70-78
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