feat: snapshot proxy #1872
feat: snapshot proxy #1872
Conversation
|
Warning Rate limit exceeded@jaybuidl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis update modifies the Solidity testing suite and introduces a new snapshot proxy contract. The test file has updated import paths and event emissions that now reference new base classes, along with an added test function for validating snapshot proxy behavior. Meanwhile, the new snapshot proxy contract enables balance queries for staked tokens and incorporates governance functions for updating the core address and governor, aligning the system with the revised Kleros V2 architecture. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
contracts/src/snapshot-proxy/KlerosCoreSnapshotProxy.sol (3)
20-24: Consider using immutable state variables.The
nameandsymbolstate variables could be made immutable since they are set at deployment and never change.Apply this diff to optimize gas usage:
- string public name = "Staked Pinakion"; - string public symbol = "stPNK"; + string public immutable name = "Staked Pinakion"; + string public immutable symbol = "stPNK";
26-29: Consider adding events for access control.The
onlyByGovernormodifier should emit events when access is denied for better transparency and monitoring.Add an event and emit it in the modifier:
+ event AccessDenied(address indexed caller, string reason); modifier onlyByGovernor() { - require(governor == msg.sender, "Access not allowed: Governor only."); + if (governor != msg.sender) { + emit AccessDenied(msg.sender, "Access not allowed: Governor only."); + revert("Access not allowed: Governor only."); + } _; }
34-37: Add events for initialization.The constructor should emit events when setting the initial governor and core contract addresses.
Add events and emit them in the constructor:
+ event GovernorChanged(address indexed previousGovernor, address indexed newGovernor); + event CoreChanged(address indexed previousCore, address indexed newCore); constructor(address _governor, IKlerosCore _core) { + emit GovernorChanged(address(0), _governor); + emit CoreChanged(address(0), address(_core)); governor = _governor; core = _core; }contracts/test/foundry/KlerosCore.t.sol (1)
1283-1308: Enhance test coverage for snapshot proxy.The test function
test_setStake_snapshotProxyCheckshould include additional test cases:
- Verify behavior when core contract is not set
- Test error handling for zero addresses
- Test balance updates after stake changes
Add these test cases to improve coverage:
function test_setStake_snapshotProxyCheck() public { vm.prank(staker1); core.setStake(GENERAL_COURT, 12346); KlerosCoreSnapshotProxy snapshotProxy = new KlerosCoreSnapshotProxy(governor, IKlerosCore(address(core))); + // Test zero address validation + vm.expectRevert("New governor cannot be zero address"); + vm.prank(governor); + snapshotProxy.changeGovernor(address(0)); + + vm.expectRevert("New core cannot be zero address"); + vm.prank(governor); + snapshotProxy.changeCore(IKlerosCore(address(0))); + + // Test balance updates after stake changes + vm.prank(staker1); + core.setStake(GENERAL_COURT, 20000); + assertEq(snapshotProxy.balanceOf(staker1), 20000, "Balance not updated after stake increase"); + // ... rest of the test }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/snapshot-proxy/KlerosCoreSnapshotProxy.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (2)
contracts/src/snapshot-proxy/KlerosCoreSnapshotProxy.sol (1)
13-15: LGTM! Clean interface definition.The interface is minimal and focused on the required functionality.
contracts/test/foundry/KlerosCore.t.sol (1)
1358-1358: LGTM! Event emission updates.The event emissions have been correctly updated to use base classes.
Also applies to: 1624-1624, 1640-1640, 1945-1945, 1954-1954, 1956-1956, 2724-2724, 2728-2728
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (2)
13-15: Add NatSpec documentation to the interface.Consider adding NatSpec documentation to the interface and its function for better code documentation.
interface IKlerosCore { + /// @dev Returns the sortition module instance used by KlerosCore. + /// @return The ISortitionModule interface of the sortition module. function sortitionModule() external view returns (ISortitionModule); }
75-77: Consider adding input validation and optimizing gas usage.
- Add zero-address validation for the input parameter.
- Use assembly to optimize the tuple destructuring if gas efficiency is a concern.
function balanceOf(address _account) external view returns (uint256 totalStaked) { + require(_account != address(0), "Account cannot be zero address"); - (totalStaked, , , ) = core.sortitionModule().getJurorBalance(_account, 0); + assembly { + // Store the function selector for getJurorBalance + mstore(0x00, 0x12345678) // Replace with actual selector + // Store the account address + mstore(0x04, _account) + // Store courtID (0) + mstore(0x24, 0) + + // Call getJurorBalance + let success := staticcall( + gas(), + // Get the sortition module address + sload(core.slot), + 0x00, + 0x44, + 0x00, + 0x20 + ) + + // Revert if call failed + if iszero(success) { + revert(0, 0) + } + + // Load the total staked amount + totalStaked := mload(0x00) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
🔇 Additional comments (2)
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (2)
34-37: LGTM!The modifier implementation is correct and includes a clear error message.
9-9: Verify compatibility with the specified Solidity version.The contract uses a very recent Solidity version (0.8.24). Ensure this version is compatible with all dependencies and deployment targets.
✅ Verification successful
🌐 Web query:
What is the latest stable version of Solidity and when was version 0.8.24 released?Length of output: 2845
Solidity Version Compatibility Verified
The contract’s
pragma solidity 0.8.24;is using a stable release that was published on January 26, 2024. Although the latest stable version is 0.8.28 (released October 9, 2024), version 0.8.24 remains in the 0.8.x series, ensuring feature compatibility with our dependencies and deployment targets. No immediate update is necessary unless you plan to leverage improvements or fixes present in later releases.
|
Code Climate has analyzed commit ff46e40 and detected 0 issues on this pull request. View more on Code Climate. |
|
PR-Codex overview
This PR introduces the
KlerosCoreSnapshotProxy, a new proxy contract for managing stakedPinakiontokens. It includes deployment scripts and updates tests to validate the proxy's functionality and governance mechanisms.Detailed summary
KlerosCoreSnapshotProxycontract for managing stakedPinakion.balanceOffunction to check staked PNK for an address.KlerosCoreSnapshotProxy.Summary by CodeRabbit
New Features
Tests
Refactor