Release v4.0.8 #1898
Release v4.0.8 #1898
Conversation
fix(web): verdict-answer-decoding
refactor(web): jurors-page-scroll
feat: snapshot proxy
fix(web): arbitraion-cost-decimals
fix: external link href typo
…ovements Refactor/sdk and voting tab improvements
WalkthroughThis pull request integrates a new contract, KlerosCoreSnapshotProxy, across various networks by adding new deployment steps, corresponding JSON deployment files, and documentation updates. It also refactors the codebase by removing extraneous metadata comments from several Solidity files, updating tests to verify proxy functionality, and enhancing the SDK and frontend utilities (including new functions like Changes
Sequence Diagram(s)sequenceDiagram
participant D as Deployer
participant DS as Deployment Script
participant P as KlerosCoreSnapshotProxy
participant T as Test Suite
D->>DS: Initiate deployArbitration process
DS->>P: Deploy KlerosCoreSnapshotProxy (with deployer and core.target)
P->>P: Constructor sets governor & core
T->>P: Call balanceOf(staker address)
P-->>T: Return staked balance
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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-neo 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-neo-devtools 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. |
✅ Deploy Preview for kleros-v2-testnet-devtools 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: 0
🧹 Nitpick comments (6)
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (1)
114-122: Consider aligning margin values for consistent spacing.The
StyledButtonhas a margin of 16px whileStyledEnsureChainhas 8px. Consider aligning these values for more consistent spacing in the UI.const StyledEnsureChain = styled(EnsureChain)` - margin: 8px auto; + margin: 16px auto; `;kleros-sdk/src/dataMappings/utils/index.ts (1)
5-6: Consider using a generic type parameter instead ofany.While the implementation is correct, using a generic type parameter would provide better type safety and maintainability.
Apply this diff to use a generic type parameter:
-export const isUndefined = (maybeObject: any): maybeObject is undefined | null => +export const isUndefined = <T>(maybeObject: T | undefined | null): maybeObject is undefined | null => typeof maybeObject === "undefined" || maybeObject === null;web-devtools/src/app/(main)/dispute-template/page.tsx (1)
203-206: Useunknowntype for error handling.Using
anytype bypasses TypeScript's type checking. Consider usingunknowntype and properly type-checking the error:- } catch (e: any) { + } catch (e: unknown) { console.error(e); - debounceErrorToast(e?.message); + debounceErrorToast(e instanceof Error ? e.message : String(e)); setDisputeDetails(undefined);web/src/pages/Cases/CaseDetails/Voting/Classic/OptionsContainer.tsx (1)
62-66: Consider addingRTAFromTemplate?.descriptionto memo dependencies.The
useMemodependency array should includeRTAFromTemplate?.descriptionto ensure the memo updates when the description changes.}, [disputeDetails]); + }, [disputeDetails?.answers]);contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (1)
51-53: Consider adding events for governance actions.It's recommended to emit events when governance parameters are changed for better transparency and off-chain tracking.
function changeGovernor(address _governor) external onlyByGovernor { + emit GovernorChanged(governor, _governor); governor = _governor; } function changeCore(IKlerosCore _core) external onlyByGovernor { + emit CoreChanged(address(core), address(_core)); core = _core; }Add event declarations at the top of the contract:
event GovernorChanged(address indexed previousGovernor, address indexed newGovernor); event CoreChanged(address indexed previousCore, address indexed newCore);Also applies to: 57-59
web/src/pages/Jurors/DisplayJurors.tsx (1)
34-34: Consider making jurorsPerPage configurable.The hardcoded value of 10 might need to be adjusted based on screen size or user preferences.
- const jurorsPerPage = 10; + const jurorsPerPage = process.env.REACT_APP_JURORS_PER_PAGE ?? 10;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
contracts/README.md(3 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(1 hunks)contracts/deploy/00-home-chain-arbitration.ts(2 hunks)contracts/deployments/arbitrum/KlerosCoreSnapshotProxy.json(1 hunks)contracts/deployments/arbitrumSepolia/KlerosCoreSnapshotProxy.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/KlerosCoreSnapshotProxy.json(1 hunks)contracts/src/arbitration/KlerosCore.sol(0 hunks)contracts/src/arbitration/KlerosCoreBase.sol(0 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(0 hunks)contracts/src/arbitration/KlerosGovernor.sol(0 hunks)contracts/src/arbitration/arbitrables/DisputeResolver.sol(0 hunks)contracts/src/arbitration/devtools/DisputeResolverRuler.sol(0 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(0 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(0 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol(0 hunks)contracts/src/arbitration/evidence/EvidenceModule.sol(0 hunks)contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol(0 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(0 hunks)contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol(1 hunks)contracts/src/gateway/ForeignGateway.sol(0 hunks)contracts/src/gateway/HomeGateway.sol(0 hunks)contracts/src/gateway/interfaces/IForeignGateway.sol(0 hunks)contracts/src/gateway/interfaces/IHomeGateway.sol(0 hunks)contracts/src/kleros-v1/interfaces/IArbitrableV1.sol(0 hunks)contracts/src/kleros-v1/interfaces/IArbitratorV1.sol(0 hunks)contracts/src/libraries/SortitionSumTreeFactory.sol(0 hunks)contracts/src/test/KlerosCoreMock.sol(0 hunks)contracts/test/foundry/KlerosCore.t.sol(10 hunks)kleros-sdk/src/dataMappings/utils/index.ts(1 hunks)kleros-sdk/src/dataMappings/utils/populateTemplate.ts(1 hunks)kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts(2 hunks)kleros-sdk/test/getDispute.test.ts(5 hunks)web-devtools/.gitignore(1 hunks)web-devtools/src/app/(main)/dispute-template/page.tsx(2 hunks)web-devtools/src/context/Web3Provider.tsx(2 hunks)web-devtools/src/hooks/queries/useDisputeTemplateFromId.ts(2 hunks)web-devtools/src/utils/isEmpty.ts(1 hunks)web/src/components/DisputePreview/DisputeContext.tsx(1 hunks)web/src/components/Verdict/FinalDecision.tsx(2 hunks)web/src/hooks/useClassicAppealContext.tsx(1 hunks)web/src/hooks/useScrollTop.ts(1 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/OptionsContainer.tsx(5 hunks)web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx(3 hunks)web/src/pages/Jurors/DisplayJurors.tsx(3 hunks)web/src/pages/Resolver/Parameters/Jurors.tsx(1 hunks)web/src/utils/getVoteChoice.ts(2 hunks)
💤 Files with no reviewable changes (20)
- contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
- contracts/src/kleros-v1/interfaces/IArbitrableV1.sol
- contracts/src/arbitration/KlerosCoreNeo.sol
- contracts/src/kleros-v1/interfaces/IArbitratorV1.sol
- contracts/src/arbitration/KlerosGovernor.sol
- contracts/src/arbitration/evidence/EvidenceModule.sol
- contracts/src/gateway/HomeGateway.sol
- contracts/src/arbitration/interfaces/IDisputeKit.sol
- contracts/src/libraries/SortitionSumTreeFactory.sol
- contracts/src/gateway/interfaces/IHomeGateway.sol
- contracts/src/arbitration/devtools/DisputeResolverRuler.sol
- contracts/src/arbitration/KlerosCore.sol
- contracts/src/arbitration/arbitrables/DisputeResolver.sol
- contracts/src/gateway/ForeignGateway.sol
- contracts/src/arbitration/evidence/ModeratedEvidenceModule.sol
- contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
- contracts/src/gateway/interfaces/IForeignGateway.sol
- contracts/src/test/KlerosCoreMock.sol
- contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol
- contracts/src/arbitration/KlerosCoreBase.sol
✅ Files skipped from review due to trivial changes (1)
- contracts/src/arbitration/dispute-kits/DisputeKitGated.sol
🧰 Additional context used
🧠 Learnings (1)
web-devtools/src/utils/isEmpty.ts (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1725
File: web/src/utils/index.ts:7-7
Timestamp: 2024-11-12T04:49:43.234Z
Learning: In `utils/index.ts`, keep the `isEmpty` function simple by accepting only strings. For null or undefined checks, users can pair it with `isUndefined`.
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-neo-devtools
- GitHub Check: Header rules - kleros-v2-neo-devtools
- GitHub Check: Pages changed - kleros-v2-neo-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (30)
web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx (2)
24-24: LGTM!The new import and styled component are well-structured and maintain consistency with the existing codebase.
Also applies to: 39-41
52-129: LGTM!The integration of
EnsureChainimproves the component's structure without impacting the core vote revealing functionality. The changes align well with the PR's objective of enhancing UI handling.web-devtools/src/utils/isEmpty.ts (1)
1-1: LGTM!The implementation is concise, follows best practices, and aligns with the retrieved learnings by keeping the function simple and accepting only strings.
web-devtools/src/hooks/queries/useDisputeTemplateFromId.ts (1)
8-8: LGTM!The additional check for empty values improves input validation and aligns with the overall goal of enhancing validation across the codebase.
Also applies to: 22-22
kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts (1)
5-5: LGTM!The change from truthy check to
isUndefinedimproves validation by explicitly checking forundefinedandnullvalues, making the behavior more predictable.Also applies to: 39-40
web-devtools/src/app/(main)/dispute-template/page.tsx (2)
29-30: LGTM!The new utility imports are well-organized and have clear, descriptive names.
198-198: LGTM!Good defensive programming practice to prevent unnecessary processing when the input is empty.
web/src/utils/getVoteChoice.ts (1)
3-3: LGTM! Improved handling of "Refuse to Arbitrate" votes.The changes enhance consistency by using the standardized title from
RefuseToArbitrateAnswerwhile maintaining backward compatibility for other vote values.Also applies to: 12-12
kleros-sdk/src/dataMappings/utils/populateTemplate.ts (1)
14-33: LGTM! Well-structured refactoring of RTA handling.The extraction of RTA logic into
findAndUpdateRTAimproves code organization while adding the ability to preserve template-specific descriptions.web/src/components/DisputePreview/DisputeContext.tsx (1)
98-101:❓ Verification inconclusive
Verify external link handling with the
toprop.The change from
hreftotosuggests theExternalLinkcomponent might be using React Router's Link. Please ensure this doesn't affect the handling of external URLs.
🏁 Script executed:
#!/bin/bash # Description: Check ExternalLink component implementation # Test: Search for ExternalLink component definition rg -A 10 "export.*ExternalLink"Length of output: 305
Action Required: Confirm External URL Behavior
The ExternalLink component is a styled version of React Router's Link, which means it expects a
toprop. Please verify that passing an external URL (e.g., viadisputeDetails?.frontendUrl) alongsidetarget="_blank"andrel="noreferrer"works as intended for external navigation. If there are issues with routing or navigation for external URLs, consider implementing conditional logic or a fallback to a standard<a>element.web/src/pages/Cases/CaseDetails/Voting/Classic/OptionsContainer.tsx (1)
91-99: LGTM! Enhanced UX with tooltips for answer descriptions.The addition of tooltips improves the user experience by making answer descriptions easily accessible.
web/src/hooks/useScrollTop.ts (1)
7-11: LGTM! Nice UX improvement.The addition of the
smoothparameter enhances the scrolling experience by allowing smooth transitions when needed.web/src/pages/Resolver/Parameters/Jurors.tsx (1)
65-65:❓ Verification inconclusive
Verify the impact of increased precision.
The arbitration fee precision has been increased from 5 to 18 decimals. While this provides more accurate representation, please ensure this change doesn't affect:
- Fee calculations in the UI
- Display formatting
- User experience (e.g., showing too many decimal places)
🏁 Script executed:
#!/bin/bash # Search for other instances of formatETH to ensure consistency rg "formatETH" -A 2Length of output: 2061
Impact Verification for Expanded Fee Precision
The arbitration fee's formatting now explicitly uses 18 decimals in Jurors.tsx, whereas other parts of the codebase rely on the default precision (usually 4). Please verify that this increased precision:
- Does not disrupt fee calculations: Ensure that using
formatETH(data ?? BigInt(0), 18)doesn’t introduce rounding or calculation inconsistencies.- Maintains consistent display formatting: Confirm that showing up to 18 decimals won’t overwhelm the UI or confuse users, especially compared to other views (like CourtOverview and CourtDetails) that format fees with fewer decimal places.
- Preserves user experience: Double-check that the higher precision aligns with overall design expectations and that any extra fractional digits are necessary and handled appropriately.
contracts/src/arbitration/view/KlerosCoreSnapshotProxy.sol (2)
28-31: LGTM! Secure governor check.The modifier correctly restricts access to governance functions.
69-71:❓ Verification inconclusive
Verify integration with Snapshot.
The
balanceOffunction uses a fixed court ID of 0. Please ensure:
- This doesn't exclude PNK staked in other courts
- The function signature matches Snapshot's requirements
- The returned balance format is compatible with Snapshot's voting power calculation
🌐 Web query:
What is the required ERC-20 interface for Snapshot voting power?💡 Result:
Snapshot's voting system primarily relies on the standard ERC-20
balanceOffunction to calculate voting power, without requiring specialized token extensions like ERC-20 Votes or Snapshots. Here's a breakdown:Core Requirements
- Standard ERC-20 Interface
- The base
balanceOf(address account)function is sufficient for most Snapshot strategies (e.g.,erc20-balance-of). Voting power is calculated using token balances at a specific block height[1][2][5].- Delegated balances are tracked separately via offchain methods (e.g., CoW DAO combines delegated/non-delegated vCOW/COW balances across Ethereum/Gnosis Chain)[1].
Advanced Use Cases
For enhanced functionality:
2. Threshold-Based Voting
Strategies likeerc20-with-balanceuse thresholds (e.g., 1 token = 1 vote) but still only requirebalanceOf[3].
- Cross-Chain Verification
Storage proofs (via Herodotus) enable verifying balances across chains without requiring token bridges[4].Onchain Governance Extensions
While not required for basic Snapshot voting:
4. ERC-20 Votes/Snapshots
- Used in onchain governance systems (e.g., OpenZeppelin’s Governor) to prevent double-spending attacks during votes[6][7][9].
- Adds delegation/snapshotting via methods like
getPastVotes(address account, uint256 blockNumber)[6][7].Summary Table
Feature Required Interface Use Case Example Basic Voting balanceOfCoW DAO’s vCOW/COW weights[1] Threshold Voting balanceOf+ strategy logic1 token = 1 vote systems[3] Cross-Chain Strategies Storage proofs + balanceOfL1 token voting on L2[4] Onchain Governance ERC-20 Votes/Snapshot extensions[6][7] Aragon/OpenZeppelin integrations Snapshot’s flexibility allows DAOs to use simple ERC-20 tokens while optionally adopting extensions like ERC-20 Votes for stricter onchain governance needs[5][6].
Citations:
- 1: https://docs.cow.fi/governance/snapshot
- 2: https://docs.snapshot.box/user-guides/voting-strategies
- 3: https://docs.snapshot.box/user-guides/spaces/space-handbook/voting-threshold
- 4: https://snapshot.mirror.xyz/F0wSmh8LROHhLYGQ7VG6VEG1_L8_IQk8eC9U7gFwep0?collectors=true
- 5: https://github.com/snapshot-labs/snapshot-docs/blob/master/snapshot-x/protocol/voting-strategies.md
- 6: https://www.aragon.org/how-to/token-standards-for-onchain-governance
- 7: https://docs.openzeppelin.com/contracts/3.x/api/token/ERC20
- 8: https://docs.openzeppelin.com/contracts/4.x/api/token/ERC20
- 9: https://forum.openzeppelin.com/t/questions-about-snapshots-and-erc20votes-voting-power-transfers/32209
- 10: https://github.com/vote-erc246/ERC-246
Verify Snapshot Integration and Court ID Behavior
The current implementation correctly exposes an ERC‐20–compliant
balanceOffunction (i.e. returning a uint256) which Snapshot strategies expect for calculating voting power. However, note that:
- The fixed court ID of 0 passed to
getJurorBalancemay exclude PNK staked in other courts. Please confirm that either court 0 is the only relevant court for Snapshot voting or that any balances in other courts are intentionally omitted.- The overall return value (from
getJurorBalance) should be verified to ensure it’s directly compatible with Snapshot’s voting power calculation.web/src/pages/Jurors/DisplayJurors.tsx (1)
64-67: LGTM! Improved pagination UX.The addition of
scrollTop(true)before navigation provides a better user experience by ensuring the new page starts at the top.web-devtools/src/context/Web3Provider.tsx (1)
9-9:✅ Verification successful
LGTM! SDK configuration is properly integrated.
The SDK configuration is correctly placed after wagmiConfig creation and uses consistent chain and transport settings.
Please verify that the SDK configuration works correctly by checking the following:
Also applies to: 83-88
🌐 Web query:
What is the latest version of @kleros/kleros-sdk and are there any breaking changes in its configuration?💡 Result:
The latest version of
@kleros/kleros-sdkis 2.2.0 according to its npm registry listing[10]. Based on the available search results:
- The SDK facilitates interactions with Kleros 2.0's decentralized dispute resolution system through modular components for contract interactions and evidence handling[4][7].
- While no explicit breaking changes are mentioned in the search results for version 2.2.0, the project maintains strict security practices including external audits and bug bounties[7]. Previous updates addressed vulnerabilities like those found in version 2.1.8[9].
For configuration details:
// Example usage pattern from Kleros v2 architecture: import { Arbitrable } from "@kleros/kleros-sdk" const arbitrable = new Arbitrable({ contractAddress: "0x988b3a5...", // Court contract address[2] provider: /* Web3 provider */, })The SDK follows semantic versioning where major versions (X.x.x) indicate potential breaking changes[6]. As this is a minor version update (2.2.0), it likely introduces backward-compatible improvements rather than configuration-breaking changes[10].
Citations:
- 1: https://github.com/kleros/kleros-v2/blob/dev/package.json
- 2: https://etherscan.io/address/0x988b3a538b618c7a603e1c11ab82cd16dbe28069
- 3: https://blog.kleros.io/kleros-2-0-beta-is-here-get-started/
- 4: https://github.com/kleros/kleros-v2
- 5: https://github.com/kleros/lifi-sdk/blob/main/CHANGELOG.md
- 6: https://www.silabs.com/developer-tools/sdk-release-and-maintenance-policy
- 7: https://blog.kleros.io/kleros-project-update-2024/
- 8: https://github.com/kleros/gtcr-sdk
- 9: https://github.com/kleros/kleros-v2/issues
- 10: https://www.npmjs.com/package/@kleros/kleros-sdk
LGTM! SDK configuration is correctly integrated.
The SDK configuration, placed right after the wagmiConfig creation, is aligned with the consistent chain and transport settings used by web3modal. The latest documentation confirms that @kleros/kleros-sdk (v2.2.0) introduces no breaking configuration changes, so the current setup is safe. Please ensure to keep an eye on future updates, but no modifications are needed at this time.
Also applies to: lines 83–88
contracts/deploy/00-home-chain-arbitration.ts (1)
13-13: LGTM! KlerosCoreSnapshotProxy deployment is properly configured.The deployment is correctly set up with the deployer address and core contract target.
Also applies to: 99-103
web/src/components/Verdict/FinalDecision.tsx (1)
96-103: LGTM! Loading state handling is improved.The changes enhance user experience by:
- Adding proper loading state handling
- Providing fallback for currentRuling
- Adding loading skeletons for better visual feedback
Also applies to: 126-130, 136-140
contracts/deploy/00-home-chain-arbitration-neo.ts (1)
126-130:✅ Verification successful
LGTM! KlerosCoreSnapshotProxy deployment matches the non-neo version.
The deployment configuration is consistent with the non-neo version and properly placed after core configuration.
Run the following script to verify deployment consistency:
🏁 Script executed:
#!/bin/bash # Description: Verify KlerosCoreSnapshotProxy deployment configuration consistency # Test: Compare deployment configurations between neo and non-neo versions rg -A 5 "KlerosCoreSnapshotProxy"Length of output: 66932
Verified deployment configuration for KlerosCoreSnapshotProxy is consistent.
The grep output confirms that both the neo and non-neo deployment scripts use the same configuration and arguments ([deployer, core.target]), properly placing the deployment after the core configuration. No changes are necessary.
kleros-sdk/test/getDispute.test.ts (2)
108-115: LGTM! Well-structured test setup.The introduction of
customRTAAnswerobject improves test readability and reusability.
149-152: LGTM! Thorough test assertions.The test properly verifies that:
- Only the title is overwritten with the standard refuse to arbitrate answer
- The description remains unchanged from the custom answer
Also applies to: 187-189
web/src/hooks/useClassicAppealContext.tsx (1)
136-137: LGTM! Improved null safety.The changes enhance robustness by:
- Adding explicit empty object check:
Object.keys(dispute).length === 0- Using optional chaining
?.for safer array accessAlso applies to: 140-141
contracts/test/foundry/KlerosCore.t.sol (1)
1283-1308: LGTM! Comprehensive test coverage for KlerosCoreSnapshotProxy.The test thoroughly verifies:
- Token properties (name, symbol, decimals)
- Governance setup and access control
- Balance tracking for staked tokens
- Access restrictions for core and governor changes
web-devtools/.gitignore (1)
49-52: LGTM! Improved environment file management.The changes enhance environment file handling by:
- Ignoring all
.env*and.flaskenv*files by default- Explicitly tracking
.env.projectand.env.vaultfor version controlcontracts/README.md (2)
22-22: New Arbitrum One Deployment Entry Added
The addition of the KlerosCoreSnapshotProxy entry with address0xEF719a5B3352F607e6C4E17b7e0cDAd8322fEC95aligns with the corresponding deployment JSON incontracts/deployments/arbitrum/KlerosCoreSnapshotProxy.json. Please verify that this address is consistently used across all deployment scripts and documentation.
43-43: New Arbitrum Sepolia Deployment Entry Added
The new entry for KlerosCoreSnapshotProxy with address0xd74e61A4dB9C6c3F2C97b62a319aE194f616858Cis correctly placed under the "Arbitrum Sepolia" subsection. Ensure that this address remains synchronized with the deployment details incontracts/deployments/arbitrumSepolia/KlerosCoreSnapshotProxy.jsonand any related scripts.contracts/deployments/arbitrumSepolia/KlerosCoreSnapshotProxy.json (1)
1-229: Deployment JSON for Arbitrum Sepolia
The JSON file comprehensively defines the deployment of the KlerosCoreSnapshotProxy contract. The ABI includes the constructor,balanceOf,changeCore,changeGovernor, and all getter functions (e.g.core,decimals,governor,name,symbol). The provided address0xd74e61A4dB9C6c3F2C97b62a319aE194f616858Cmatches the documentation. Everything appears consistent and complete.contracts/deployments/arbitrumSepoliaDevnet/KlerosCoreSnapshotProxy.json (1)
1-229: Deployment JSON for Arbitrum Sepolia Devnet
This deployment file for the Devnet includes all necessary details (ABI, transaction receipt, metadata, and storage layout) and specifies the contract’s address as0x9300D415af6e747ADe3C6cbA09a3b3CD5fb0c091. The constructor arguments also appear correct, with the governor and core addresses provided (note the core address is0xA54e7A16d7460e38a8F324eF46782FB520d58CE8). Please double-check that the core address corresponds to your intended devnet configuration.contracts/deployments/arbitrum/KlerosCoreSnapshotProxy.json (1)
1-155: Deployment JSON for Arbitrum Main Deployment
The JSON file defines the deployment details for KlerosCoreSnapshotProxy on Arbitrum with address0xEF719a5B3352F607e6C4E17b7e0cDAd8322fEC95. The ABI and included metadata accurately reflect the public functions and constructor of the contract. This file is consistent with your README documentation and deployment scripts.
fix: toastify styling tweak to allow for clicking on navbar while act…
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/src/styles/global-style.ts (1)
128-131: Consider refining the toast container styling approach.The current implementation has a few potential issues:
- The attribute selector might be too broad
- Using
!importantmakes future style overrides difficult- Removing the
topposition without a replacement could affect toast visibilityConsider these improvements:
- [class*="Toastify__toast-container"] { + .Toastify__toast-container--top-right, + .Toastify__toast-container--top-left, + .Toastify__toast-container--top-center { top: unset; - padding-top: 20px !important; + padding-top: 20px; }Also, consider adding a comment explaining why these positioning changes are necessary for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/src/components/Field.tsx(1 hunks)web/src/styles/global-style.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- web/src/components/Field.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: SonarCloud
- GitHub Check: Scorecards analysis
- GitHub Check: Analyze (javascript)
chore(web): migrate-to-reown
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
web/src/pages/GetPnk/WalletProvider.tsx (2)
22-27: Initialize WagmiAdapter carefully
You're setting the adapter'snetworksto[mainnet], but in other lines, you use Arbitrum or Arbitrum Sepolia as the default network. If you intend to support multiple networks, consider including them here or confirm thatallowUnsupportedChainmeets your needs.
35-46: Check mismatch in createAppKit configuration
createAppKitsets[mainnet]innetworkswhile defaulting to Arbitrum or Arbitrum Sepolia. This works withallowUnsupportedChain: true, but may confuse end users if a default network is not explicitly in thenetworkslist. Consider adding Arbitrum or Arbitrum Sepolia to thenetworksarray to provide a more consistent user experience.web-devtools/src/context/Web3Provider.tsx (2)
82-87: Enhance SDK configuration robustness.The SDK configuration could benefit from error handling and validation.
Consider wrapping the configuration in a try-catch block:
+try { configureSDK({ client: { chain: isProduction ? arbitrum : arbitrumSepolia, transport: transports[isProduction ? arbitrum.id : arbitrumSepolia.id], }, }); +} catch (error) { + console.error('Failed to configure SDK:', error); + throw new Error('SDK configuration failed'); +}
15-19: Enhance environment variable validation.Consider using a more descriptive error message and validating the API key format.
const alchemyApiKey = process.env.NEXT_PUBLIC_ALCHEMY_API_KEY; -if (!alchemyApiKey) { +if (!alchemyApiKey || !/^[A-Za-z0-9_-]+$/.test(alchemyApiKey)) { throw new Error( - "Alchemy API key is not set in NEXT_PUBLIC_ALCHEMY_API_KEY environment variable." + "Invalid or missing Alchemy API key in NEXT_PUBLIC_ALCHEMY_API_KEY environment variable. Please ensure it contains only alphanumeric characters, underscores, and hyphens." ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
kleros-app/README.md(1 hunks)kleros-app/package.json(2 hunks)kleros-app/src/lib/atlas/providers/AtlasProvider.tsx(3 hunks)web-devtools/package.json(2 hunks)web-devtools/src/components/ConnectWallet/index.tsx(2 hunks)web-devtools/src/consts/chains.ts(1 hunks)web-devtools/src/context/Web3Provider.tsx(2 hunks)web-devtools/src/utils/getDisputeRequestParamsFromTxn.ts(1 hunks)web/package.json(2 hunks)web/src/components/ConnectWallet/index.tsx(2 hunks)web/src/consts/chains.ts(1 hunks)web/src/consts/eip712-messages.ts(2 hunks)web/src/context/AtlasProvider.tsx(1 hunks)web/src/context/Web3Provider.tsx(5 hunks)web/src/pages/GetPnk/WalletProvider.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
web/src/context/AtlasProvider.tsx (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1687
File: web/src/context/AtlasProvider.tsx:225-244
Timestamp: 2024-11-12T04:49:48.060Z
Learning: In `web/src/context/AtlasProvider.tsx`, the `atlasUri` variable comes from environment variables and does not change, so it does not need to be included in dependency arrays.
web-devtools/src/context/Web3Provider.tsx (1)
Learnt from: alcercu
PR: kleros/kleros-v2#1703
File: web/src/context/Web3Provider.tsx:9-9
Timestamp: 2024-11-12T04:49:43.234Z
Learning: The SDK structure will be improved in PR #1704.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
- GitHub Check: contracts-testing
🔇 Additional comments (38)
web/src/consts/eip712-messages.ts (2)
25-25: LGTM!The formatting change maintains code readability while preserving the correct EIP-712 message structure and type safety.
44-44: LGTM!The formatting change maintains code readability while preserving the correct EIP-712 message structure and type safety.
web/package.json (5)
82-82: New Dependency Addition: @bigmi/reactThe addition of
@bigmi/reactat version^0.1.0is noted. Please ensure that this library is required and integrated within the project as expected, and that its version aligns with the overall React ecosystem in use.
90-93: Dependency Updates and Additions: LIFI & Reown AppKitThese lines update the version of
@lifi/wallet-managementto^3.6.0and@lifi/widgetto^3.14.2, while also adding@reown/appkitand@reown/appkit-adapter-wagmiat version^1.6.5. This aligns with the integration of Reown AppKit for enhanced wallet connection management. Please verify that the updated versions are fully compatible with the project's requirements and that the new dependencies are used consistently in the codebase.
96-96: New Dependency Addition: @solana/wallet-adapter-reactThe dependency
@solana/wallet-adapter-reacthas been added at version^0.15.35. Ensure this addition is intentional for handling Solana wallet interactions and that its integration is tested in relevant parts of the application.
100-100: Dependency Version Update: @wagmi/coreThe version for
@wagmi/corehas been updated to^2.16.3. Confirm that this update does not introduce breaking changes and is compatible with the other Wagmi packages used in the project.
129-129: Dependency Version Update: wagmiThe update of
wagmito version^2.14.10should be reviewed to ensure that it includes the expected enhancements and that no new incompatibilities have been introduced. Testing around wallet integrations and other Wagmi-related functionality is recommended.web-devtools/src/components/ConnectWallet/index.tsx (2)
36-37:✅ Verification successful
LGTM! Verify the hook usage pattern.
The migration from web3modal to appkit hooks looks good. The API usage pattern appears to be compatible.
Let's verify the correct usage of these hooks in the codebase:
Also applies to: 44-44
🏁 Script executed:
#!/bin/bash # Description: Check the usage pattern of appkit hooks across the codebase. # Search for useAppKit usage echo "Checking useAppKit usage pattern..." rg -A 2 "useAppKit\(\)" # Search for useAppKitState usage echo -e "\nChecking useAppKitState usage pattern..." rg -A 2 "useAppKitState\(\)" # Search for open method calls with view parameter echo -e "\nChecking open method calls..." rg "open\(\s*\{\s*view\s*:"Length of output: 1430
LGTM! Hook usage across the codebase has been verified.
- The migration from web3modal to appkit hooks is correctly implemented in both
web-devtools/src/components/ConnectWallet/index.tsxandweb/src/components/ConnectWallet/index.tsx.- Both the
useAppKit()anduseAppKitState()hooks are used with the expected pattern.- The
openmethod is consistently invoked with theviewparameter (e.g.,open({ view: "Connect" })).
3-3:❓ Verification inconclusive
Verify the @reown/appkit package installation.
The code imports hooks from the
@reown/appkitpackage. Please ensure that this package is properly installed and listed in the project's dependencies.Run the following script to check the package installation:
🏁 Script executed:
#!/bin/bash # Description: Check if @reown/appkit is properly installed. # Check package.json for the dependency echo "Checking package.json for @reown/appkit..." cat package.json | jq '.dependencies["@reown/appkit"] // .devDependencies["@reown/appkit"]' # Check if the package is installed in node_modules echo -e "\nChecking node_modules for @reown/appkit..." fd -t d '^@reown' node_modulesLength of output: 427
Action Required: Verify the Installation of
@reown/appkitThe code now imports
useAppKitanduseAppKitStatefrom@reown/appkit/react, but our initial check did not find any reference to@reown/appkitin yourpackage.jsondependencies. Additionally, the script was unable to search fornode_modulesdue to the directory not being present in the current context. Please ensure that:
- Dependency Listing: The
@reown/appkitpackage is explicitly listed in your dependency management (e.g., inpackage.jsonunderdependenciesordevDependencies).- Lock Files Verification: If the package is installed as expected, verify its presence in your lock files (like
yarn.lockorpackage-lock.json).- Installation Check: Confirm that the installation is correct (you might need to run a manual or scripted check in the environment where
node_modulesis available).Once confirmed, this import statement will integrate correctly. Please verify these points and update the dependency configuration if needed.
web/src/components/ConnectWallet/index.tsx (4)
3-3: LGTM!The migration from
@web3modal/wagmi/reactto@reown/appkit/reactis correctly implemented.
37-49: LGTM!The migration to AppKit hooks is correctly implemented. The component:
- Uses the new
useAppKitanduseAppKitStatehooks.- Correctly handles the modal's open state.
- Properly configures the connect view.
51-59: LGTM!The component maintains correct wallet connection state handling and seamlessly integrates with the updated child components.
13-14:✅ Verification successful
Verify the
isLoadingproperty in the latest version ofwagmi.The TODO comment suggests potential API changes. Let's verify if
isLoadinghas been replaced withisPendingin the latest version.
🌐 Web query:
Has wagmi replaced isLoading with isPending in useSwitchChain hook?💡 Result:
Based on recent documentation updates and community discussions around Wagmi v2.x.x (current as of 2025),
isLoadingremains present alongsideisPendingin some hooks likeuseSwitchNetwork, while newer patterns showstatus: "pending"being used instead of separate boolean flags[1][7]. However:
useSwitchNetworkstill exposesisLoading
The original implementation continues usingisLoadingto indicate network switching progress[1][3]:const { chains, error, isLoading } = useSwitchNetwork()Newer hooks like
useWaitForTransactionReceiptfavorisPending
Recent additions align with React Query v5 patterns whereisPendingreplacesisLoading[4][5]:const { data: receipt } = useWaitForTransactionReceipt({ hash, query: { enabled: !!hash } })Confusion stems from overlapping terminology
isLoading: Legacy term indicating initial fetch stateisPending: Newer term encompassing all async statesstatus: "pending": Alternative pattern via state enums[7]Current recommendation: Check both properties until full migration completes:
const { switchNetwork, error, status, // Legacy pattern ↓ isLoading, // Newer pattern ↓ pendingChainId } = useSwitchNetwork()This dual approach handles transitional APIs while maintaining backward compatibility[3][9].
Citations:
- 1: https://1.x.wagmi.sh/react/hooks/useSwitchNetwork
- 2: https://egghead.io/lessons/react-use-ispending-isloading-and-isfetching-flags-with-react-query
- 3: https://web3auth.io/community/t/not-able-to-switch-chain-using-wagmi-useswitchnetwork-hook/6840
- 4: https://wagmi.sh/react/api/hooks/useWaitForTransactionReceipt
- 5: Why do the docs recommend `isPending` instead of `isLoading`? TanStack/query#6297
- 6: https://wagmi.sh/react/api/hooks/useToken
- 7: https://wagmi.sh/react/api/hooks/useSwitchChain
- 8: https://docs.zerodev.app/react/use-session-kernelclient
- 9: Cannot switch chain using mobile browsers wevm/wagmi#4205
- 10: https://wagmi.sh/react/guides/send-transaction
Correctness Confirmed:
useSwitchChain'sisLoadingProperty Remains ValidBased on the latest documentation and community discussions for Wagmi v2.x (2025), the
useSwitchChainhook still supports theisLoadingproperty. Although some newer hooks (e.g.,useWaitForTransactionReceipt) adopt theisPendingpattern or status enums, the current implementation ofuseSwitchChaincorrectly continues to exposeisLoading. No changes are required for this file.
- The
isLoadingproperty remains active and is not replaced byisPendingforuseSwitchChain.- Some hooks in Wagmi are moving toward newer async state indicators, but these changes are not yet applicable to
useSwitchChain.web/src/pages/GetPnk/WalletProvider.tsx (3)
5-7: Using @reown/appkit
The newly introduced imports look valid, provided that all required dependencies are included in your package.json.
10-10: Confirm wagmi version compatibility
Ensure that the version of thewagmipackage is compatible with@reown/appkit-adapter-wagmito avoid potential dependency or peer version conflicts.
51-51: WagmiProvider usage looks good
Passingadapter.current.wagmiConfigtoWagmiProvideris consistent with the new setup.web/src/context/Web3Provider.tsx (8)
3-14: Adopting @reown/appkit
These imports from@reown/appkit/*andwagmialign with the latest approach for network and adapter configuration.
43-49: Possible type inconsistency in alchemyURL
AllowingchainIdto benumber | stringis flexible, but ensure that you handle string IDs correctly inalchemyToViemChain[chainId], as it expects numerical keys. Consider parsing or validating string inputs to avoid look-up failures.
51-53: Validate getChainRpcUrl
Similarly, verify that passing a stringchainIdtogetChainRpcUrlproperly resolves network addresses in all usage contexts (e.g., if the string does not match an expected ID).
60-63: Check for undefined RPC URLs
When usingfallback([http(...), webSocket(...)]), ensure each chain has validrpcUrls.default.httpandrpcUrls.default.webSocketentries; otherwise, some calls may fail at runtime.
76-76: Casting ALL_CHAINS
CastingALL_CHAINSto[AppKitNetwork, ...AppKitNetwork[]]will throw a runtime error ifALL_CHAINSis empty. Confirm thatALL_CHAINShas at least one element.
84-88: Initialize WagmiAdapter
This setup correctly passesnetworks,projectId, andtransports. Confirm all your networks’ transport URLs are consistently defined.
97-116: createAppKit configuration
This is a solid approach to unify chain, adapter, and default network configurations. The commented block withinfeaturesis a good placeholder for future toggles.
118-118: Returning WagmiProvider
UsingwagmiAdapter.wagmiConfigin the<WagmiProvider>is consistent and should enable your app’s wagmi-based features properly.web/src/context/AtlasProvider.tsx (4)
2-2: No changes needed on the empty line
This line change appears to be only whitespace and has no functional impact.
3-4: Import useConfig from wagmi
Bringing inuseConfigensures you can access and propagate Wagmi’s global configuration within the AtlasProvider.
8-8: Calling useConfig
DefiningwagmiConfigby callinguseConfig()is a straightforward way to integrate wagmi settings here. Just confirm that it does not repeatedly trigger re-renders if its value changes frequently.
10-10: Include wagmiConfig in AtlasProvider
PassingwagmiConfigtoconfigallows_AtlasProviderto leverage wagmi’s settings for things like account management. This approach looks consistent with the overall architecture.web-devtools/src/consts/chains.ts (1)
1-1: LGTM! Type updates align with AppKit integration.The migration from
ChaintoAppKitNetworktype is consistent and maintains type safety while integrating with the AppKit library.Also applies to: 7-7, 11-11
web/src/consts/chains.ts (1)
25-29: Verify type compatibility in getChain function.The function returns
Chain | nullbut usesALL_CHAINSwhich containsAppKitNetworktypes. This might lead to type mismatches.Consider updating the return type to match the new AppKitNetwork type:
-export const getChain = (chainId: number): Chain | null => +export const getChain = (chainId: number): AppKitNetwork | null =>web-devtools/src/utils/getDisputeRequestParamsFromTxn.ts (1)
4-4: LGTM! Clean migration to wagmiAdapter.The update from
wagmiConfigtowagmiAdapter.wagmiConfigis consistent with the AppKit integration while maintaining proper error handling.Also applies to: 10-10
web-devtools/src/context/Web3Provider.tsx (1)
76-80: LGTM! WagmiAdapter initialization is well-structured.The adapter initialization with networks, projectId, and transports is clean and type-safe.
kleros-app/src/lib/atlas/providers/AtlasProvider.tsx (2)
5-5: LGTM! Proper integration of Wagmi configuration.The addition of the
Configtype and thewagmiConfigproperty to theAtlasConfiginterface ensures proper typing and configuration support for Wagmi v2.Also applies to: 53-57
60-61: LGTM! Consistent configuration usage across hooks.The hooks are properly configured with the wagmiConfig, maintaining consistency across the provider.
Also applies to: 70-70
kleros-app/package.json (1)
52-53: LGTM! Consistent version management.The wagmi dependency is properly added to devDependencies and updated in peerDependencies with matching versions.
Also applies to: 65-65
web-devtools/package.json (2)
53-54: LGTM! New dependencies for enhanced wallet connection handling.The addition of @reown/appkit and its wagmi adapter provides improved wallet connection capabilities.
57-57:❓ Verification inconclusive
Verify compatibility with updated wagmi versions.
The updates to @wagmi/core and wagmi versions should be tested to ensure compatibility with the new appkit integration.
Also applies to: 73-73
🏁 Script executed:
#!/bin/bash # Check for any breaking changes or compatibility issues in the changelog curl -s "https://raw.githubusercontent.com/wagmi-dev/wagmi/main/CHANGELOG.md" | grep -A 10 "## 2.14.0"Length of output: 102
Action Required: Verify @wagmi/core Update Compatibility
The dependency update to
"@wagmi/core": "^2.16.3"(and similarly for line 73) appears correct, but the automated check for breaking changes via the changelog returned no output. This result is inconclusive, so please manually confirm that there are no breaking changes between the previously used version and 2.16.3—especially regarding the new appkit integration.
- Action Items:
- Manually inspect the wagmi changelog for any breaking changes from version 2.14.0 up to 2.16.3.
- Conduct integration tests or other manual validations to ensure compatibility with the updated dependency.
kleros-app/README.md (1)
30-31: LGTM! Documentation properly updated.The README has been correctly updated to reflect the new wagmiConfig requirement in the AtlasProvider configuration.
Also applies to: 35-36, 40-40
PR-Codex overview
This PR primarily introduces a new
KlerosCoreSnapshotProxycontract, enhances various components with improved functionality, and refines the integration of thewagmilibrary for better state management in the application.Detailed summary
isEmptyutility function inweb-devtools/src/utils/isEmpty.ts.isUndefinedutility function inkleros-sdk/src/dataMappings/utils/index.ts.KlerosCoreSnapshotProxycontract for managing staked PNK.balanceOffunction inKlerosCoreSnapshotProxyto return staked amount.wagmiintegration in multiple components for improved state management.arbitrationFeeformatting inweb/src/pages/Resolver/Parameters/Jurors.tsx.Summary by CodeRabbit
New Features
Tests
Chores / Refactor