feat: snapshot proxy #1872
feat: snapshot proxy #1872
Conversation
WalkthroughA new Solidity proxy contract ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Proxy as SnapshotProxy
participant Core as IKlerosCore
participant Sortition as SortitionModule
Caller->>Proxy: balanceOf(account)
Proxy->>Core: sortitionModule()
Core-->>Proxy: Return SortitionModule instance
Proxy->>Sortition: getJurorBalance(0, account)
Sortition-->>Proxy: Return staked balance
Proxy-->>Caller: Return staked balance
sequenceDiagram
participant Gov as Governor
participant Proxy as SnapshotProxy
Gov->>Proxy: changeGovernor(newGovernor)
Proxy-->>Gov: Governor updated
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 failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-testnet failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-university ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Code Climate has analyzed commit a703fdd and detected 0 issues on this pull request. View more on Code Climate. |
|
![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
| function changeGovernor(address _governor) external onlyByGovernor { | ||
| governor = _governor; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add zero-address validation in admin functions.
The changeGovernor and changeCore functions should validate that the new addresses are not zero addresses.
Apply this diff to add validation:
function changeGovernor(address _governor) external onlyByGovernor {
+ require(_governor != address(0), "New governor cannot be zero address");
+ emit GovernorChanged(governor, _governor);
governor = _governor;
}
function changeCore(IKlerosCore _core) external onlyByGovernor {
+ require(address(_core) != address(0), "New core cannot be zero address");
+ emit CoreChanged(address(core), address(_core));
core = _core;
}Also applies to: 47-49
| function balanceOf(address _account) external view returns (uint256 totalStaked) { | ||
| (totalStaked, , , ) = core.sortitionModule().getJurorBalance(_account, 0); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling for external calls.
The balanceOf function should handle potential failures from external calls to the core contract.
Apply this diff to add validation and error handling:
function balanceOf(address _account) external view returns (uint256 totalStaked) {
+ require(_account != address(0), "Account cannot be zero address");
+ require(address(core) != address(0), "Core contract not set");
+ ISortitionModule sortitionModule = core.sortitionModule();
+ require(address(sortitionModule) != address(0), "Sortition module not set");
(totalStaked, , , ) = core.sortitionModule().getJurorBalance(_account, 0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function balanceOf(address _account) external view returns (uint256 totalStaked) { | |
| (totalStaked, , , ) = core.sortitionModule().getJurorBalance(_account, 0); | |
| } | |
| function balanceOf(address _account) external view returns (uint256 totalStaked) { | |
| require(_account != address(0), "Account cannot be zero address"); | |
| require(address(core) != address(0), "Core contract not set"); | |
| ISortitionModule sortitionModule = core.sortitionModule(); | |
| require(address(sortitionModule) != address(0), "Sortition module not set"); | |
| (totalStaked, , , ) = core.sortitionModule().getJurorBalance(_account, 0); | |
| } |
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
PR-Codex overview
This PR introduces the
KlerosCoreSnapshotProxycontract, enabling Snapshot voting by exposing the staked PNK balance. It also updates the test suite to validate the new functionality and adjusts some imports.Detailed summary
KlerosCoreSnapshotProxycontract with functions for managing governance and querying staked PNK.balanceOffunction to retrieve staked PNK for an address.KlerosCore.t.solto verify the new proxy's behavior and governance functionality.Summary by CodeRabbit
New Features
Tests