Release of v2-testnet-4.0.0 #1709
Conversation
refactor(web-devtools): revamp-ruler-ui
…ent hook, with help from green
…, calculation changes
feat: add four new stat variables, stake simulator
feat: update footer socials
…ate-contact-support-link chore(web): remove-unused-libs-and-update-contact-support-link
fix(Governor): remove CappedMath and tx order
feat: tweak in court stake display in dashboard
Signed-off-by: StepSecurity Bot <[email protected]>
…sion-update Chore(hardhat)/hardhat ether version update
…ion_1730800738 [StepSecurity] ci: Harden GitHub Actions
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (33)
contracts/deploy/utils/index.ts (2)
Line range hint
1-28: Consider implementing the TODO comment to improve maintainability.Instead of hardcoding chain IDs in enums, deriving them from
hardhat.configandviem/chainswould be more maintainable and less error-prone.Would you like me to help create a GitHub issue to track this technical debt and propose an implementation using
viem/chains?
Line range hint
29-39: Enhance error handling and type safety in network validation functions.While the functions are logically correct, there are opportunities for improvement:
- Consider throwing an error instead of logging to console in
isSkipped- Add type safety for the
skipparameterConsider this implementation:
export const isMainnet = (network: Network) => network.tags.production ?? false; export const isDevnet = (network: Network) => network.name.endsWith("Devnet"); -export const isSkipped = async (network: Network, skip: boolean) => { +export const isSkipped = async (network: Network, skip: boolean): Promise<boolean> => { if (skip) { - console.error(`Error: incompatible network ${network.name} for this deployment script`); + throw new Error(`Incompatible network ${network.name} for this deployment script`); } return false; };.github/workflows/dependabot-automerge.yml (1)
Line range hint
33-39: Consider adding branch protection verification.While the auto-merge configuration is correct, it might be worth adding a check to verify that required branch protection rules are in place before enabling auto-merge.
- name: Enable auto-merge for Dependabot PRs if: ${{ steps.metadata.outputs.update-type == 'version-update:semver-minor' || steps.metadata.outputs.update-type == 'version-update:semver-patch' }} + run: | + # Verify branch protection rules + BRANCH_PROTECTION=$(gh api repos/${{ github.repository }}/branches/main/protection --jq '.required_status_checks != null') + if [ "$BRANCH_PROTECTION" = "true" ]; then + gh pr merge --auto --rebase "$PR_URL" + else + echo "Warning: Branch protection rules not properly configured" + exit 1 + fi - run: gh pr merge --auto --rebase "$PR_URL" env: PR_URL: ${{github.event.pull_request.html_url}} GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}}contracts/deploy/00-home-chain-pnk-faucet.ts (3)
Line range hint
11-27: Consider making the faucet amount configurableThe faucet amount is hardcoded to 10000 tokens. Consider making this configurable through environment variables or deployment configuration to support different environments and testing scenarios.
Example improvement:
+ const FAUCET_AMOUNT = process.env.PNK_FAUCET_AMOUNT || "10000"; - await execute("PNKFaucet", { from: deployer, log: true }, "changeAmount", hre.ethers.parseUnits("10000", "ether")); + await execute("PNKFaucet", { from: deployer, log: true }, "changeAmount", hre.ethers.parseUnits(FAUCET_AMOUNT, "ether"));Enhance error handling for missing PNK address
The current implementation silently skips deployment if the PNK address is not found for the chain. Consider adding explicit error handling to make troubleshooting easier.
Example improvement:
const pnkAddress = pnkByChain.get(chainId); - if (pnkAddress) { + if (!pnkAddress) { + throw new Error(`No PNK token address configured for chain ${HomeChains[chainId]} (${chainId})`); + } await deploy("PNKFaucet", {
Line range hint
31-34: Document skip conditionsThe skip configuration would benefit from a comment explaining the conditions under which the deployment is skipped. This would help other developers understand the deployment flow better.
Example improvement:
+ // Skip deployment if: + // 1. The network is specifically marked to be skipped, or + // 2. The network's chainId is not in the HomeChains enum deployArbitration.skip = async ({ network }) => { return isSkipped(network, !HomeChains[network.config.chainId ?? 0]); };
Since we found only one instance of a placeholder address in the deployment scripts, and it's the same one we're reviewing, I can generate the final response:
Replace placeholder PNK token address for Arbitrum Sepolia
The placeholder text "INSERT ARBITRUM SEPOLIA PNK TOKEN ADDRESS HERE" in
contracts/deploy/00-home-chain-pnk-faucet.tsneeds to be replaced with the actual PNK token address for Arbitrum Sepolia. Deploying with a placeholder address will cause deployment failures.🔗 Analysis chain
Line range hint
4-8: Replace placeholder PNK token address for Arbitrum SepoliaThe placeholder text "INSERT ARBITRUM SEPOLIA PNK TOKEN ADDRESS HERE" needs to be replaced with the actual PNK token address for Arbitrum Sepolia. Deploying with a placeholder address will cause deployment failures.
Let's verify if this placeholder exists in other deployment scripts:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar placeholder patterns in deployment scripts rg -i "INSERT.*ADDRESS.*HERE" "contracts/deploy/"Length of output: 177
contracts/deploy/utils/getContractOrDeploy.ts (2)
10-19: Consider enhancing error handling.While the type safety improvements are good, consider adding try-catch blocks to handle potential deployment failures gracefully.
export const getContractOrDeploy = async ( hre: HardhatRuntimeEnvironment, contractName: string, options: DeployOptions ): Promise<Contract> => { + try { let contract = await hre.ethers.getContractOrNull<Contract>(contractName); if (!contract) { console.log(`contract ${contractName} not deployed, deploying now...`); await hre.deployments.deploy(contractName, options); contract = await hre.ethers.getContract<Contract>(contractName); } else { console.log(`contract ${contractName} already deployed`); } return contract; + } catch (error) { + console.error(`Failed to deploy or retrieve contract ${contractName}:`, error); + throw error; + } };
Line range hint
21-34: Consider refactoring to reduce code duplication.The new function shares significant logic with
getContractOrDeploy. Consider extracting the common logic into a shared helper function.+async function getOrDeployContract( + hre: HardhatRuntimeEnvironment, + contractName: string, + deployFn: () => Promise<void>, + isUpgradeable: boolean +): Promise<Contract> { + try { + let contract = await hre.ethers.getContractOrNull<Contract>(contractName); + if (!contract) { + console.log( + `contract ${contractName} not deployed, deploying${ + isUpgradeable ? " as upgradable" : "" + } now...` + ); + await deployFn(); + contract = await hre.ethers.getContract<Contract>(contractName); + } else { + console.log(`contract ${contractName} already deployed`); + } + return contract; + } catch (error) { + console.error(`Failed to deploy or retrieve contract ${contractName}:`, error); + throw error; + } +} export const getContractOrDeploy = async ( hre: HardhatRuntimeEnvironment, contractName: string, options: DeployOptions -): Promise<Contract> => { - let contract = await hre.ethers.getContractOrNull<Contract>(contractName); - if (!contract) { - console.log(`contract ${contractName} not deployed, deploying now...`); - await hre.deployments.deploy(contractName, options); - contract = await hre.ethers.getContract<Contract>(contractName); - } else { - console.log(`contract ${contractName} already deployed`); - } - return contract; +): Promise<Contract> => { + return getOrDeployContract( + hre, + contractName, + () => hre.deployments.deploy(contractName, options), + false + ); }; export const getContractOrDeployUpgradable = async ( hre: HardhatRuntimeEnvironment, contractName: string, options: DeployOptions -): Promise<Contract> => { - let contract = await hre.ethers.getContractOrNull<Contract>(contractName); - if (!contract) { - console.log(`contract ${contractName} not deployed, deploying as upgradable now...`); - await deployUpgradable(hre.deployments, contractName, options); - contract = await hre.ethers.getContract<Contract>(contractName); - } else { - console.log(`contract ${contractName} already deployed`); - } - return contract; +): Promise<Contract> => { + return getOrDeployContract( + hre, + contractName, + () => deployUpgradable(hre.deployments, contractName, options), + true + ); };contracts/deploy/05-arbitrable-dispute-template.ts (2)
36-37: Consider enhancing event logging.While the type checking is good, the current logging could be improved:
- Add error handling for failed transactions
- Log specific event fields rather than dumping all args
- Add structured logging with event names and relevant context
Consider this enhancement:
- tx?.logs?.forEach((event) => { - if (event instanceof EventLog) console.log("event: %O", event.args); - }); + if (!tx) { + throw new Error("Transaction failed"); + } + tx.logs?.forEach((event) => { + if (event instanceof EventLog) { + console.log({ + eventName: event.eventName, + blockNumber: event.blockNumber, + args: event.args, + }); + } + });
Configuration values need to be externalized and TODOs need to be addressed
The verification reveals several important findings:
- The IPFS content is accessible (HTTP 200)
- The arbitrator address
0xD08Ab...917bis consistently used across multiple configuration files, suggesting it's a valid deployment address- Multiple TODO comments exist throughout deploy scripts, including the specific
disputeTemplateMapping: TODOmentioned in the review- There's no environment variable usage in deploy scripts, indicating configuration values are indeed hardcoded
Action items:
- Extract hardcoded values to a configuration file following the project's pattern (found in
contracts/hardhat.config.ts)- Address the
disputeTemplateMapping: TODOcomment which appears in multiple deploy scripts- Document the configuration values in the deployment documentation
🔗 Analysis chain
Line range hint
9-31: Address hardcoded configuration and TODO.The template configuration has several items that need attention:
- There's a TODO comment that needs to be addressed
- Contains hardcoded chain IDs and contract addresses
- References external IPFS content that should be verified
Let's verify the IPFS content and contract addresses:
Consider:
- Moving configuration to environment variables or a config file
- Adding validation for chain IDs and addresses
- Documenting the source and purpose of the IPFS content
Would you like help creating a configuration system for these values?
Let me gather more information about any configuration patterns in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify IPFS content availability curl -s "https://ipfs.io/ipfs/QmXDiiBAizCPoLqHvcfTzuMT7uvFEe1j3s4TgoWWd4k5np/proof-of-humanity-registry-policy-v1.3.pdf" -o /dev/null -w "%{http_code}\n" # Search for environment configuration files that might contain these addresses fd -t f -e .env -e .json -e .yaml -e .yml | xargs rg "0xc5e9ddebb09cd64dfacab4011a0d5cedaf7c9bdb|0xD08Ab99480d02bf9C092828043f611BcDFEA917b"Length of output: 1588
Script:
#!/bin/bash # Check for environment variable usage in deploy scripts rg "process\.env" contracts/deploy/ # Look for configuration files fd "config|\.env" -t f # Check for any TODO comments in the deploy scripts rg -i "todo" contracts/deploy/Length of output: 3719
contracts/deploy/upgrade-kleros-core.ts (1)
29-34: Consider using a named constant for the sortition module parameterWhile the update to
ethers.toBeHex()is good, the hardcoded value5would be clearer as a named constant explaining its purpose in the sortition module configuration.+const DEFAULT_SORTITION_K_VALUE = 5; [ minStake, alpha, feeForJuror, 256 ], [0, 0, 0, 10], -ethers.toBeHex(5), +ethers.toBeHex(DEFAULT_SORTITION_K_VALUE), sortitionModule.address,contracts/deploy/02-home-gateway-to-ethereum.ts (1)
Line range hint
1-1: Consider tracking the TODO comment about deterministic deploymentsThe TODO comment about using deterministic deployments should be addressed as it could improve deployment consistency across networks.
Would you like me to create a GitHub issue to track the implementation of deterministic deployments? This would help ensure consistent contract addresses across different networks.
contracts/deploy/utils/deployTokens.ts (3)
6-17: Consider adding error handling.While the function logic is sound, it would benefit from proper error handling to gracefully handle deployment failures.
Consider wrapping the deployments in try-catch:
export const deployERC20AndFaucet = async ( hre: HardhatRuntimeEnvironment, deployer: string, ticker: string, faucetFundingAmount: bigint = hre.ethers.parseUnits("100000") ): Promise<Contract> => { - const erc20 = await deployERC20(hre, deployer, ticker); - if (!isMainnet(hre.network)) { - await deployFaucet(hre, deployer, ticker, erc20, faucetFundingAmount); + try { + const erc20 = await deployERC20(hre, deployer, ticker); + if (!isMainnet(hre.network)) { + await deployFaucet(hre, deployer, ticker, erc20, faucetFundingAmount); + } + return erc20; + } catch (error) { + console.error(`Failed to deploy ${ticker} token:`, error); + throw error; } - return erc20; };
32-53: Consider emitting events and extracting magic numbers.The function logic is solid, but could be improved in two ways:
- Emit events for significant state changes (funding the faucet).
- Extract the magic number (5n) into a named constant.
Consider these improvements:
+const FAUCET_MINIMUM_BALANCE_THRESHOLD_DENOMINATOR = 5n; export const deployFaucet = async ( hre: HardhatRuntimeEnvironment, deployer: string, ticker: string, erc20: Contract, faucetFundingAmount: bigint ): Promise<void> => { const faucet = await getContractOrDeploy(hre, `${ticker}Faucet`, { from: deployer, contract: "Faucet", args: [erc20.target], log: true, }); const faucetBalance = await erc20.balanceOf(faucet.target); const deployerBalance = await erc20.balanceOf(deployer); - if (deployerBalance >= faucetFundingAmount && faucetBalance < faucetFundingAmount / 5n) { + if (deployerBalance >= faucetFundingAmount && + faucetBalance < faucetFundingAmount / FAUCET_MINIMUM_BALANCE_THRESHOLD_DENOMINATOR) { console.log(`funding ${ticker}Faucet with ${faucetFundingAmount}`); await erc20.transfer(faucet.target, faucetFundingAmount); + // Consider emitting an event here if the contract supports it } };
55-67: Consider adding JSDoc documentation.The function would benefit from documentation explaining its purpose and parameters.
Consider adding JSDoc:
+/** + * Deploys a test ERC721 token contract + * @param hre Hardhat Runtime Environment + * @param deployer Address of the deployer + * @param name Name of the token + * @param ticker Symbol of the token + * @returns The deployed ERC721 contract instance + */ export const deployERC721 = async ( hre: HardhatRuntimeEnvironment, deployer: string, name: string, ticker: string ): Promise<Contract> => {.github/workflows/contracts-testing.yml (2)
77-84: Consider optimizing test execution performance.While the current configuration is correct, for large test suites, you might benefit from parallelizing the test execution.
Consider adding the following parameters to the hardhat coverage command:
-yarn hardhat coverage --solcoverjs ./.solcover.js --temp artifacts --testfiles './test/**/*.ts' --show-stack-traces +yarn hardhat coverage --solcoverjs ./.solcover.js --temp artifacts --testfiles './test/**/*.ts' --show-stack-traces --parallel --max-workers 4This can significantly reduce the execution time of your test suite.
🧰 Tools
🪛 yamllint
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 83-83: trailing spaces
(trailing-spaces)
45-48: Remove trailing spaces.There are trailing spaces on these lines that should be removed for consistency.
🧰 Tools
🪛 yamllint
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
contracts/deploy/04-foreign-arbitrable.ts (1)
Line range hint
52-56: Consider making gas fees configurableHardcoding gas fees to ONE_GWEI could cause deployment failures under varying network conditions. Consider making these values configurable based on network conditions or deployment environment.
Suggestions:
- Add gas fee configuration to deployment parameters
- Use network-specific default values
- Consider implementing dynamic gas fee estimation
contracts/deploy/fix1148.ts (1)
Line range hint
44-50: Consider tracking the dispute kit disabling limitationThe code correctly enables the new dispute kit across courts, but there's a noted limitation preventing the disabling of old dispute kits (referenced in the comment). Consider creating an issue to track this architectural limitation for future improvement.
Would you like me to help create a GitHub issue to track this limitation?
contracts/deploy/01-foreign-gateway-on-ethereum.ts (3)
10-11: LGTM: Correct migration to ethers.js v6 utility functions.The changes correctly update the imports to use the new ethers.js v6 API (
zeroPadValue,toBeHex).Consider adding a comment explaining that these utilities are the v6 replacements for
hexZeroPadandhexlifyto help with future maintenance.
22-22: LGTM: Correct provider instantiation for ethers.js v6.The provider instantiation is correctly updated to use
ethers.JsonRpcProvider.Consider adding error handling for provider initialization:
- const homeChainProvider = new ethers.JsonRpcProvider(homeNetwork.url); + const homeChainProvider = await ethers.JsonRpcProvider.fromConnection( + homeNetwork.url, + undefined, + { staticNetwork: true } // Optimization: prevents network detection calls + ).catch((error) => { + throw new Error(`Failed to connect to home network at ${homeNetwork.url}: ${error.message}`); + });
Line range hint
41-45: Address critical TODOs before deployment.Several important initialization steps are marked as TODO:
- Gateway initialization and fee configuration
- Dispute creator fund management
- Court fee setup
These unimplemented features could lead to operational issues if not addressed before deployment.
Would you like me to help create a GitHub issue to track these TODOs and provide implementation suggestions?
contracts/deploy/01-foreign-gateway-on-gnosis.ts (2)
Line range hint
47-54: Address security concerns in gateway initialization.The TODO comments indicate potential security issues:
- The gateway remains enabled during initialization
- Fee configuration is incomplete for multiple courts
Consider:
- Adding a temporary pause mechanism during initialization
- Implementing proper fee validation before enabling dispute creation
- Adding explicit checks for fee configuration completeness
Would you like me to help create a more secure initialization sequence or generate a GitHub issue to track these security concerns?
Based on the gathered information, I can now provide the final response:
Consider using hardhat-deploy's built-in deterministic deployment feature instead of nonce-based address prediction.
The project already uses
hardhat-deploywhich supports deterministic deployments through thedeterministicDeploymentoption. The current nonce-based prediction is fragile and could break if deployment sequence changes.
- In
contracts/deploy/01-foreign-gateway-on-gnosis.ts, replace the nonce-based prediction with deterministic deployment:await deployUpgradable(deployments, "ForeignGatewayOnGnosis", { deterministicDeployment: true, // Add this line from: deployer, contract: "ForeignGateway", ... });🔗 Analysis chain
Line range hint
18-30: Consider implementing deterministic deployments.The current address prediction mechanism relies on nonce sequencing, which could be fragile if:
- Multiple deployments occur in parallel
- A transaction fails and needs to be retried
- The deployment sequence changes
Consider implementing one of these alternatives:
- Use CREATE2 for deterministic addresses
- Implement the
hardhat-deploydeterministic deployment feature- Use a deployment proxy pattern that maintains stable addresses
Let me gather more information about the deployment setup to provide a more comprehensive verification.
Let me check the actual deployment implementation to understand the current setup better.
Let me check the hardhat configuration to see if there are any deployment plugins or settings that could help with deterministic deployments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the codebase already has deterministic deployment implementations rg -g '!node_modules' -g '*.{ts,js}' 'deterministic.*deploy' .Length of output: 499
Script:
#!/bin/bash # Check for CREATE2 or proxy deployment patterns rg -g '!node_modules' -g '*.{ts,js}' 'CREATE2|ProxyAdmin|TransparentUpgradeableProxy' -A 3 # Check hardhat config for deployment plugins cat hardhat.config.ts || cat hardhat.config.js # Look for deployment patterns in package.json cat package.jsonLength of output: 2902
Script:
#!/bin/bash # Check the deployment utility implementation cat contracts/deploy/utils/deployUpgradable.ts # Check for any existing deployment configuration fd -e json -e js -e ts 'hardhat.config' contracts/ # Look for deployment scripts that might show the deployment pattern cat contracts/deploy/01-foreign-gateway-on-gnosis.tsLength of output: 5256
Script:
#!/bin/bash # Check hardhat configuration cat contracts/hardhat.config.ts # Check if there are any deployment-related dependencies in contracts/package.json cat contracts/package.jsonLength of output: 15408
contracts/deploy/03-vea-mock.ts (2)
84-84: Document the purpose of the zero address parameter.Consider adding a comment explaining the purpose of this zero address parameter in the ArbitrableExample constructor.
Line range hint
1-93: Consider addressing TODOs and documenting magic numbers.The script contains several areas that need attention:
- Multiple TODO comments need to be addressed before production deployment
- The nonce calculation (
nonce += 3) should be documented to explain the deployment sequence- The hardcoded
extraDatavalue needs proper implementation as noted in the commentWould you like help implementing any of these improvements or creating GitHub issues to track them?
contracts/deploy/00-home-chain-arbitration.ts (2)
66-66: Document the significance of jurorsForCourtJump value.The value 256 for
jurorsForCourtJumpseems to be a magic number. Consider adding a comment explaining why this specific value was chosen and its implications for the court system.
94-96: Document currency rate values and precision.The hard-coded rate values (12225583, 60327783) and their precision (12, 11) should be documented. Consider:
- Adding comments explaining how these rates were determined
- Moving these values to a configuration file
- Documenting the implications of these rates on the system
+ // Rate values based on market prices as of <DATE> + // PNK: 1 ETH = 12225.583 PNK (12 decimals precision) + // DAI: 1 ETH = 6032.7783 DAI (11 decimals precision) await changeCurrencyRate(core, await pnk.getAddress(), true, 12225583, 12); await changeCurrencyRate(core, await dai.getAddress(), true, 60327783, 11); await changeCurrencyRate(core, await weth.getAddress(), true, 1, 1);contracts/deploy/04-klerosliquid-to-v2-gnosis.ts (3)
12-12: Consider adding a comment explaining the gas price constant.The ONE_GWEI constant is used throughout the deployment for gas price settings. Consider adding a comment explaining why this specific value was chosen and its implications for deployment costs.
-const ONE_GWEI = BigNumber.from(parseUnits("1", "gwei")); +// Fixed gas price for predictable deployment costs +const ONE_GWEI = BigNumber.from(parseUnits("1", "gwei")); // 1 GWEI = 1e9 wei
Line range hint
44-51: Consider parameterizing deployment configuration values.The deployment script uses hardcoded values for critical parameters:
- minStakingTime: 99999999
- minStake: 9999999 ETH
- jurorsForCourtJump: 9999999
These values seem unrealistic and could cause issues in production. Consider:
- Moving these to a configuration file
- Adding environment-specific values
- Adding validation checks
Would you like me to help create a configuration structure for these parameters?
Line range hint
108-143: Implement or remove the TODO mock deployment section.The commented-out mock deployment code is important for local testing but currently inactive. Consider:
- Implementing the mock deployment for the Hardhat network
- Moving the chain-specific addresses to a configuration file
- Adding proper error messages for unsupported networks
Would you like me to help implement the mock deployment functionality or create a GitHub issue to track this task?
contracts/deploy/00-home-chain-arbitration-ruler.ts (1)
10-57: Consider adding more logging statements.To improve the observability and debuggability of the deployment process, consider adding more logging statements at key points in the script. This could include logging the addresses of the deployed contracts, the values of important parameters, and the success or failure of critical operations.
For example, you could add logging statements like:
console.log("Deployed PNK token at address:", pnk.target); console.log("Deployed KlerosCoreRuler at address:", core.target); console.log("Successfully changed currency rates for PNK, DAI, and WETH");contracts/deploy/00-home-chain-arbitration-neo.ts (1)
107-111: Improve error handling and logging.The error handling for the
changeCurrencyRatefunction call can be improved:
- Log the error message along with any relevant context for easier debugging.
- Consider rethrowing the error or gracefully handling the failure based on the severity and impact on the deployment process.
Apply this diff to improve the error handling:
try { await changeCurrencyRate(core, await weth.getAddress(), true, 1, 1); } catch (e) { - console.error("failed to change currency rates:", e); + console.error(`Failed to change currency rates for WETH. Error: ${e.message}`); + // Rethrow the error or handle it gracefully based on the severity and impact + throw e; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
.github/workflows/contracts-testing.yml(3 hunks).github/workflows/dependabot-automerge.yml(1 hunks).github/workflows/sonarcloud.yml(1 hunks)contracts/deploy/00-home-chain-arbitration-neo.ts(1 hunks)contracts/deploy/00-home-chain-arbitration-ruler.ts(1 hunks)contracts/deploy/00-home-chain-arbitration-university.ts(1 hunks)contracts/deploy/00-home-chain-arbitration.ts(4 hunks)contracts/deploy/00-home-chain-pnk-faucet.ts(2 hunks)contracts/deploy/01-foreign-gateway-on-ethereum.ts(3 hunks)contracts/deploy/01-foreign-gateway-on-gnosis.ts(3 hunks)contracts/deploy/02-home-gateway-to-ethereum.ts(1 hunks)contracts/deploy/03-vea-mock.ts(4 hunks)contracts/deploy/04-foreign-arbitrable.ts(3 hunks)contracts/deploy/04-klerosliquid-to-v2-gnosis.ts(2 hunks)contracts/deploy/05-arbitrable-dispute-template.ts(2 hunks)contracts/deploy/fix1148.ts(2 hunks)contracts/deploy/upgrade-kleros-core.ts(3 hunks)contracts/deploy/utils/deployTokens.ts(1 hunks)contracts/deploy/utils/getContractOrDeploy.ts(1 hunks)contracts/deploy/utils/index.ts(3 hunks)contracts/deploy/utils/klerosCoreHelper.ts(1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/contracts-testing.yml
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
🔇 Additional comments (48)
contracts/deploy/utils/klerosCoreHelper.ts (2)
1-2: LGTM! Clean and appropriate imports.
The imports are well-organized and use modern ethers v6 utilities.
4-20: 🛠️ Refactor suggestion
Essential improvements needed for production readiness.
While the core logic is sound, several critical improvements are needed:
- Error handling for blockchain interactions
- Production-grade logging
- Gas optimization for multiple transactions
- Input validation for rate parameters
Consider this improved implementation:
export const changeCurrencyRate = async (
core: KlerosCore | KlerosCoreNeo | KlerosCoreRuler | KlerosCoreUniversity,
erc20: string,
accepted: boolean,
rateInEth: BigNumberish,
rateDecimals: BigNumberish
) => {
+ // Input validation
+ if (toBigInt(rateDecimals) < 0n) {
+ throw new Error("Rate decimals must be non-negative");
+ }
+
+ try {
const pnkRate = await core.currencyRates(erc20);
+ const updates = [];
if (pnkRate.feePaymentAccepted !== accepted) {
- console.log(`core.changeAcceptedFeeTokens(${erc20}, ${accepted})`);
- await core.changeAcceptedFeeTokens(erc20, accepted);
+ updates.push(core.changeAcceptedFeeTokens.populateTransaction(erc20, accepted));
}
if (pnkRate.rateInEth !== toBigInt(rateInEth) || pnkRate.rateDecimals !== rateDecimals) {
- console.log(`core.changeCurrencyRates(${erc20}, ${rateInEth}, ${rateDecimals})`);
- await core.changeCurrencyRates(erc20, rateInEth, rateDecimals);
+ updates.push(core.changeCurrencyRates.populateTransaction(erc20, rateInEth, rateDecimals));
}
+
+ // Execute all updates in a single multicall if possible
+ if (updates.length > 0) {
+ const transactions = await Promise.all(updates);
+ if ('multicall' in core) {
+ await core.multicall(transactions.map(tx => tx.data));
+ } else {
+ // Fall back to sequential execution
+ for (const tx of transactions) {
+ await core.signer.sendTransaction(tx);
+ }
+ }
+ }
+ } catch (error) {
+ throw new Error(`Failed to update currency rate for ${erc20}: ${error.message}`);
+ }
};Key improvements:
- Added input validation for rate parameters
- Implemented error handling with meaningful error messages
- Optimized gas usage by batching transactions when possible
- Removed console.log in favor of proper error propagation
Let's verify if the contracts support multicall:
.github/workflows/sonarcloud.yml (3)
10-12: LGTM: Explicit permission declarations follow security best practices.
The addition of explicit minimal permissions both at workflow and job level follows the principle of least privilege, which is a security best practice for GitHub Actions.
Also applies to: 15-17
21-24: LGTM: Harden Runner addition enhances security.
The addition of the Harden Runner step with egress policy audit is a good security practice as it helps monitor and control the workflow's network access.
26-26: LGTM: Action versions pinned to specific commit hashes.
Pinning action versions to specific commit hashes instead of using floating tags (v3 or master) is a security best practice as it prevents supply chain attacks through action version changes.
Let's verify the pinned commit hashes are from the official repositories:
Also applies to: 31-31
✅ Verification successful
Verified: Action commit hashes match official releases
The commit hashes in the workflow file correspond to official releases from their respective repositories:
- actions/checkout@f43a0e5 → Release 3.6.0
- SonarSource/sonarcloud-github-action@9f9bba2 → Official SonarSource commit
- step-security/harden-runner@91182cc → Release v2.10.1
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the commit hashes belong to the official repositories
# and match the stated versions
# Test: Verify actions/checkout commit
gh api repos/actions/checkout/commits/f43a0e5ff2bd294095638e18286ca9a3d1956744 --jq '.commit.message'
# Test: Verify SonarSource/sonarcloud-github-action commit
gh api repos/SonarSource/sonarcloud-github-action/commits/9f9bba2c7aaf7a55eac26abbac906c3021d211b2 --jq '.commit.message'
# Test: Verify step-security/harden-runner commit
gh api repos/step-security/harden-runner/commits/91182cccc01eb5e619899d80e4e971d6181294a7 --jq '.commit.message'
Length of output: 483
.github/workflows/dependabot-automerge.yml (3)
6-8: LGTM! Following security best practices with least privilege principle.
The addition of top-level read-only permissions aligns with GitHub's security best practices by restricting default access.
Line range hint 19-24: LGTM! Strong security controls for network access.
The harden-runner configuration with blocked egress and specific allowed endpoints provides excellent network security controls.
Line range hint 6-24: Verify the completeness of required permissions.
The workflow's permission setup looks good, implementing a secure configuration with:
- Default read-only access at workflow level
- Explicit required permissions at job level
- Hardened runner with restricted egress
However, let's verify if any additional permissions might be needed for the workflow's operations.
✅ Verification successful
The workflow's permissions are correctly configured and complete
The current permission setup is appropriate and complete for all operations performed in the workflow:
contents: writeis needed for rebasing PRs during auto-mergepull-requests: writeis required for enabling auto-merge and accessing PR metadata- The workflow uses
dependabot/fetch-metadataaction andgh pr mergecommand, both of which are covered by these permissions - The hardened runner correctly allows only
api.github.com:443which is sufficient for GitHub API operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential GitHub API calls in the workflow that might need additional permissions
# Test 1: Look for GitHub API interactions in the workflow
rg -l 'github\.' .github/workflows/dependabot-automerge.yml
# Test 2: Check for other workflow files that might have similar permission needs
fd -e yml -e yaml . .github/workflows/ --exec rg -l 'dependabot' {}
Length of output: 249
Script:
#!/bin/bash
# Let's examine the actual GitHub API interactions and permissions used in both files
# Check the complete content of dependabot-automerge.yml
echo "=== Content of dependabot-automerge.yml ==="
cat .github/workflows/dependabot-automerge.yml
# Check the complete content of pr-labels.yml for any dependabot-related permissions
echo -e "\n=== Content of pr-labels.yml ==="
cat .github/workflows/pr-labels.yml
# Look specifically for GitHub API calls or permission requirements
echo -e "\n=== GitHub API interactions in both files ==="
rg -A 3 'github\.' .github/workflows/dependabot-automerge.yml .github/workflows/pr-labels.yml
Length of output: 4334
contracts/deploy/utils/getContractOrDeploy.ts (2)
3-4: LGTM! Import changes are appropriate.
The switch to importing Contract from 'ethers' instead of '@ethersproject/contracts' aligns with best practices, and the new import for deployUpgradable is necessary for the upgradeable contract functionality.
Line range hint 1-34: Verify deployment patterns across the codebase.
Let's ensure these changes align with existing deployment patterns and error handling.
✅ Verification successful
The initial results show consistent usage of these deployment utilities across the codebase, but we need to verify the error handling and implementation details to ensure the changes are safe and follow existing patterns.
Based on the results, I can now provide a final response as we have sufficient information about the implementation and usage patterns.
The deployment utility changes are well-structured and follow existing patterns
The implementation shows:
- Proper handling of upgradeable contracts with UUPS proxy pattern
- Consistent deployment options handling
- No direct error throwing, relying on the underlying deployment framework's error handling
The changes in getContractOrDeploy.ts align with the existing deployment patterns and maintain consistency with the rest of the codebase. The added getContractOrDeployUpgradable function follows the same pattern as the original function while properly integrating with the deployUpgradable utility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check deployment patterns and error handling in deployment scripts
echo "Checking deployment patterns..."
rg -A 2 "getContractOrDeploy|deployUpgradable" --type ts
echo "Checking error handling patterns in deployment scripts..."
rg -A 2 "try.*catch" --type ts contracts/deploy/
Length of output: 17867
Script:
#!/bin/bash
# Let's check for error handling patterns in the deployment utilities
echo "Checking error handling in deployment utilities..."
rg -A 5 "throw|Error|catch" contracts/deploy/utils/
# Let's also check the implementation of deployUpgradable
echo "\nChecking deployUpgradable implementation..."
cat contracts/deploy/utils/deployUpgradable.ts
# Let's verify the usage patterns
echo "\nChecking usage patterns of deployment functions..."
rg -A 3 "getContractOrDeployUpgradable|getContractOrDeploy" --type ts contracts/deploy/0
Length of output: 2732
contracts/deploy/05-arbitrable-dispute-template.ts (1)
4-4: LGTM! Good type safety practice.
The addition of EventLog import enhances type safety in event handling.
contracts/deploy/upgrade-kleros-core.ts (1)
8-8: LGTM: Updated to use ZeroAddress from ethers.js v6
The change from AddressZero to ZeroAddress aligns with the latest ethers.js v6 conventions.
contracts/deploy/02-home-gateway-to-ethereum.ts (1)
35-35: LGTM! Proper migration to ethers.ZeroAddress
The change from ethers.constants.AddressZero to ethers.ZeroAddress is correct and follows the latest ethers.js conventions.
contracts/deploy/utils/deployTokens.ts (2)
1-5: LGTM! Clean and well-organized imports.
The imports are properly structured and all dependencies are necessary for the implementation.
19-30: Skip comment as past review is still valid.
The previous review comment about removing redundant return await is still applicable.
.github/workflows/contracts-testing.yml (2)
Line range hint 27-40: Security hardening looks good!
The workflow has been properly secured with:
- Explicit read-only permissions
- Comprehensive egress policy with well-defined allowed endpoints
🧰 Tools
🪛 yamllint
[warning] 26-26: wrong indentation: expected 6 but found 4
(indentation)
Line range hint 50-60: Cache configuration is well structured.
Good use of:
- Versioned cache keys with secret for manual cache busting
- Proper fallback strategy with restore-keys
- Comprehensive path inclusion
🧰 Tools
🪛 yamllint
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
contracts/deploy/04-foreign-arbitrable.ts (1)
3-3: LGTM: Import changes align with ethers.js v6 best practices
The updates to import statements correctly reflect the new package structure in ethers.js v6, and the addition of BigNumber from @ethersproject/bignumber ensures precise numerical operations.
Also applies to: 6-6
contracts/deploy/fix1148.ts (4)
10-10: LGTM: Correct usage of ethers v6 ZeroAddress constant
The update from AddressZero to ZeroAddress aligns with ethers v6 naming conventions.
36-41: LGTM: Proper verification of dispute kit assignments
The assertions correctly verify the dispute kit assignments using proper address comparisons and provide clear error messages.
Line range hint 52-55: LGTM: Clear deployment configuration
The deployment configuration is well-structured with appropriate tagging and skip conditions.
19-30: Verify the intended use of ZeroAddress in constructor
The address handling with .getAddress() is correct and follows best practices. However, please verify that using ZeroAddress as the second constructor argument is intentional for the initial deployment.
Let's check the contract's constructor requirements:
contracts/deploy/01-foreign-gateway-on-ethereum.ts (1)
32-32: LGTM: Correct chain ID conversion using new utility functions.
The chain ID conversion is correctly updated to use zeroPadValue and toBeHex.
Let's verify that the chain ID conversion produces the same output as before:
✅ Verification successful
LGTM: Chain ID conversion is consistent across the codebase
The conversion pattern zeroPadValue(toBeHex(chainId), 32) is consistently used across all deployment scripts:
- contracts/deploy/01-foreign-gateway-on-ethereum.ts
- contracts/deploy/01-foreign-gateway-on-gnosis.ts
- contracts/deploy/03-vea-mock.ts
No legacy hexZeroPad usage was found, confirming the codebase has been fully migrated to the new ethers.js v6 functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new chain ID conversion produces the same output format
# Test: Search for any other chain ID conversions in the codebase for comparison
rg -A 2 'zeroPadValue.*toBeHex.*chainId'
rg -A 2 'hexZeroPad.*hexlify.*chainId'
Length of output: 83
Script:
#!/bin/bash
# Let's try a broader search for chain ID conversions and bytes32 patterns
# Search for chain ID related code
rg -A 3 'chainId.*bytes32|bytes32.*chainId' --type ts
# Search for zeroPadValue usage patterns
rg -A 2 'zeroPadValue' --type ts
# Search for hexZeroPad usage patterns (old ethers v5)
rg -A 2 'hexZeroPad' --type ts
Length of output: 1733
contracts/deploy/01-foreign-gateway-on-gnosis.ts (3)
1-10: LGTM! Import statements and constant definition are properly updated.
The changes correctly migrate to ethers v6 import style and ensure proper typing of the gas fee constant.
12-16: LGTM! Deployment function setup is well-structured.
The code properly handles both named accounts and local testing scenarios, with correct usage of ethers v6 utilities.
Line range hint 36-45: LGTM! Chain ID formatting and deployment configuration are correct.
The update to use zeroPadValue and toBeHex properly handles the chain ID conversion to bytes32.
contracts/deploy/03-vea-mock.ts (4)
60-60: LGTM: Correct removal of unnecessary await.
The removal of await is appropriate as connect is a synchronous operation.
51-52: Verify zero address handling in the HomeGateway contract.
While the comment confirms the intentional use of ZeroAddress, we should verify that the HomeGateway contract properly handles a zero address fee token to prevent potential issues.
#!/bin/bash
# Search for zero address checks in the HomeGateway contract
ast-grep --pattern 'require($_token != address(0), $_)'32-32: Verify chain ID padding for cross-chain compatibility.
The chain ID padding looks correct, but let's ensure the byte length is appropriate for cross-chain message passing.
#!/bin/bash
# Search for other chain ID handling patterns to ensure consistency
rg "chainId.*[Bb]ytes32|[Bb]ytes32.*chainId" -g "*.ts"14-14: LGTM: Proper migration to ethers.js v6 utilities.
The change correctly updates to the new ethers.js v6 utility functions.
Let's verify no old ethers.utils references remain:
✅ Verification successful
The previous command might have failed due to the dot in the pattern. Let's try a broader search to catch any ethers-related patterns and specifically look for the old utility function names that were migrated.
Based on the search results, I can now generate the final response:
Migration to ethers.js v6 utilities is complete
The codebase has been properly migrated to ethers.js v6 utilities. No instances of old ethers.utils.hexZeroPad or ethers.utils.hexlify were found, and all ethers usage follows the v6 pattern where utility functions are available directly from the ethers namespace.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining ethers.utils references that need migration
rg "ethers\.utils\." -g "*.ts"
Length of output: 151
Script:
#!/bin/bash
# Search for any remaining ethers.utils references with broader patterns
rg "ethers\." -A 2 -g "*.ts"
# Also search for specific old utility functions that were commonly used
rg "hexZeroPad|hexlify" -g "*.ts"
Length of output: 60510
contracts/deploy/00-home-chain-arbitration.ts (2)
87-89: Previous review comment is still valid.
The missing 'await' issue in the dk.changeCore call needs to be addressed as it could lead to unhandled promises.
71-71: Verify governor settings for production deployment.
Setting the governor to deployer might be suitable for testing but should be carefully reviewed for production deployment. Consider if this should be a different address in production environments.
contracts/deploy/04-klerosliquid-to-v2-gnosis.ts (2)
1-1: LGTM: Import changes align with ethers v6.
The updates to the import statements follow the recommended patterns for ethers v6, improving maintainability.
Also applies to: 6-6
Line range hint 65-95: Add deployment verification and error handling.
The xKlerosLiquidV2 initialization contains critical parameters but lacks:
- Error handling for failed transactions
- Event verification after initialization
- Parameter validation before deployment
Consider adding post-deployment verification:
const tx = await execute(
"xKlerosLiquidV2",
{
from: deployer,
log: true,
maxFeePerGas: ONE_GWEI,
maxPriorityFeePerGas: ONE_GWEI,
},
"initialize",
// ... existing parameters ...
);
// Wait for confirmation and verify events
const receipt = await tx.wait();
const initEvent = receipt.events?.find(e => e.event === 'Initialize');
if (!initEvent) throw new Error('Initialization failed - event not emitted');contracts/deploy/00-home-chain-arbitration-ruler.ts (4)
23-26: ****
Consider externalizing configuration parameters
The parameters minStake, alpha, feeForJuror, and jurorsForCourtJump are hardcoded. For improved maintainability and flexibility, consider moving these values to a configuration file or environment variables. This would allow for easier adjustments without modifying the codebase.
38-44: ****
Enhance error handling in the try-catch block
Currently, if updating the currency rates fails, the exception is logged but not handled further. Consider implementing additional error handling mechanisms, such as rethrowing the error after logging or introducing retry logic, to ensure that failures are addressed appropriately.
1-64: LGTM!
The deployment script follows best practices and is well-structured. It effectively utilizes the Hardhat runtime environment and helper functions to deploy the necessary contracts for the Kleros arbitration system. The script is also properly tagged and includes a skip condition based on the network configuration.
46-50: Verify the DisputeTemplateRegistry deployment.
The script deploys the DisputeTemplateRegistry contract using the getContractOrDeployUpgradable utility function. However, it's important to ensure that the deployment was successful and that the contract is properly initialized.
Run the following script to verify the deployment:
✅ Verification successful
Based on the results, I can see that the DisputeTemplateRegistry contract is being deployed through the getContractOrDeployUpgradable utility function in multiple deployment scripts, with proper initialization arguments. The contract is used as a dependency by other contracts like DisputeResolver variants. The deployment utility function has proper error handling and logging.
The deployment is properly implemented and follows the upgradeable pattern. The deployment script:
- Checks if contract exists
- If not, deploys as upgradeable
- Initializes with the deployer address as governor
- Returns the deployed contract instance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the `DisputeTemplateRegistry` contract deployment.
# Test: Search for the contract deployment transaction.
# Expect: A transaction deploying the `DisputeTemplateRegistry` contract.
rg -A 10 $'Deploying \'DisputeTemplateRegistry\''
# Test: Check if the `DisputeTemplateRegistry` contract is registered in the Hardhat artifacts.
# Expect: The `DisputeTemplateRegistry` artifact file exists.
fd DisputeTemplateRegistry.json ./artifacts/contracts
Length of output: 219
Script:
#!/bin/bash
# Let's verify the contract existence and deployment setup from multiple angles
# Check if the contract source exists
fd DisputeTemplateRegistry.sol
# Look for contract definition
ast-grep --pattern 'contract DisputeTemplateRegistry'
# Check deployment utility function
rg -A 5 "getContractOrDeployUpgradable"
# Look for any references to DisputeTemplateRegistry in deployment scripts
rg "DisputeTemplateRegistry"
Length of output: 540389
Script:
#!/bin/bash
# Let's verify the DisputeTemplateRegistry contract and its deployment
# Check the contract source code
ast-grep --pattern 'contract DisputeTemplateRegistry'
# Look for the deployment utility function implementation
rg -A 10 "getContractOrDeployUpgradable"
# Check if there are any deployment logs or events related to DisputeTemplateRegistry
rg -A 5 "Deploying.*DisputeTemplateRegistry"
# Check if the contract is being used by other contracts
ast-grep --pattern 'IDisputeTemplateRegistry'
Length of output: 7466
contracts/deploy/00-home-chain-arbitration-university.ts (5)
17-19: Potential undefined value in HomeChains[chainId]
The previous comment about handling a possible undefined value in HomeChains[chainId] is still applicable. This may lead to unexpected logs or errors if chainId is not present in HomeChains.
44-47: Avoid hardcoding configuration values
The earlier suggestion to avoid hardcoding minStake, alpha, feeForJuror, and jurorsForCourtJump remains relevant. Moving these values to a configuration file or defining them as constants at the top level will enhance maintainability.
73-79: Review error handling when changing currency rates
The previous concern about the error handling in the changeCurrencyRate calls is still valid. Catching errors and only logging them may allow the deployment to continue despite crucial failures.
96-98: Potential undefined value in HomeChains[network.config.chainId]
The existing comment regarding the possible undefined value in the skip function's logic is still pertinent. This could cause incorrect skip behavior during deployment.
14-14:
Correct the import of ZeroAddress
ZeroAddress is not directly available from hre.ethers; it should be accessed via hre.ethers.constants.
Apply this diff to fix the import:
- const { ZeroAddress } = hre.ethers;
+ const { ZeroAddress } = hre.ethers.constants;Likely invalid or redundant comment.
contracts/deploy/00-home-chain-arbitration-neo.ts (5)
50-55: ****
The past review comment about the potential issue with nonce-based address calculation is still valid and applicable to the current code segment.
103-103: ****
The past review comment about adding error handling for the changeCore function call is still valid and applicable to the current code segment.
127-127: ****
The past review comment about including error handling when updating the arbitrable whitelist is still valid and applicable to the current code segment.
1-135: LGTM!
The deployment script looks good overall. It follows a clear structure, handles various scenarios, and includes necessary error handling and logging. The usage of helper functions and utilities enhances code readability and reusability.
Some minor suggestions for further improvement:
- Consider adding more descriptive comments or documentation to explain the purpose and flow of the deployment process.
- Ensure consistent formatting and naming conventions throughout the script.
- Review the necessity of each deployed contract and configuration to optimize gas usage and deployment efficiency.
Great job on the comprehensive deployment script!
131-133: Verify the isSkipped function usage.
Ensure that the isSkipped function is correctly used and returns the expected result based on the provided network configuration.
Run the following script to verify the isSkipped function usage:
✅ Verification successful
Let me analyze the isSkipped function usage based on the search results.
The search results show:
- The
isSkippedfunction is defined incontracts/deploy/utils/index.ts - It takes two parameters:
networkandskip - The implementation checks if
skipis true, logs an error message, and returns true; otherwise returns false - In the file under review (
00-home-chain-arbitration-neo.ts), it's used with!HomeChains[network.config.chainId ?? 0]as the skip condition - This usage pattern is consistent across all deployment scripts, where each script checks if the current chain is supported using either
HomeChainsorForeignChains
Let me verify the HomeChains enum to ensure the condition is properly constructed.
The isSkipped function is correctly used in the deployment script
The verification confirms that:
- The
isSkippedfunction is properly implemented to check network compatibility HomeChainsenum correctly defines the supported chains (Arbitrum One: 42161, Arbitrum Sepolia: 421614)- The skip condition
!HomeChains[network.config.chainId ?? 0]will:- Return
true(skip) for unsupported chains - Return
false(don't skip) for chains defined inHomeChains
- Return
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the `isSkipped` function usage in the deployment script.
# Test: Search for the `isSkipped` function usage. Expect: Only the correct usage.
rg -A 5 $'isSkipped'
Length of output: 16408
Script:
#!/bin/bash
# Search for HomeChains definition
rg -A 10 "enum HomeChains" contracts/
Length of output: 1848
| export const PNK = (n: number) => ethers.parseUnits(String(n)); | ||
| export const ETH = (n: number) => ethers.parseUnits(String(n)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance token conversion functions with proper decimal handling and documentation.
The current implementation could benefit from:
- Explicit decimal places specification
- Input validation
- JSDoc documentation
Consider this implementation:
-export const PNK = (n: number) => ethers.parseUnits(String(n));
-export const ETH = (n: number) => ethers.parseUnits(String(n));
+/** Converts a number to PNK units (18 decimals) */
+export const PNK = (n: number): bigint => {
+ if (n < 0) throw new Error("PNK amount cannot be negative");
+ return ethers.parseUnits(String(n), 18);
+};
+
+/** Converts a number to ETH units (18 decimals) */
+export const ETH = (n: number): bigint => {
+ if (n < 0) throw new Error("ETH amount cannot be negative");
+ return ethers.parseUnits(String(n), 18);
+};📝 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.
| export const PNK = (n: number) => ethers.parseUnits(String(n)); | |
| export const ETH = (n: number) => ethers.parseUnits(String(n)); | |
| /** Converts a number to PNK units (18 decimals) */ | |
| export const PNK = (n: number): bigint => { | |
| if (n < 0) throw new Error("PNK amount cannot be negative"); | |
| return ethers.parseUnits(String(n), 18); | |
| }; | |
| /** Converts a number to ETH units (18 decimals) */ | |
| export const ETH = (n: number): bigint => { | |
| if (n < 0) throw new Error("ETH amount cannot be negative"); | |
| return ethers.parseUnits(String(n), 18); | |
| }; |
| const minStake = 2n * 10n ** 20n; | ||
| const alpha = 10000; | ||
| const feeForJuror = BigNumber.from(10).pow(17); | ||
| const feeForJuror = 10n * 17n; |
There was a problem hiding this comment.
💡 Codebase verification
From the search results, I found that:
ETH()is defined incontracts/deploy/utils/index.tsas:export const ETH = (n: number) => ethers.parseUnits(String(n));- Production deployments use
ETH(0.1)which equals10n ** 17n(0.1 ETH) - The test files consistently use
10n * 17nwhich is incorrect as it equals 170 wei
The fee calculation is incorrect and should be 10n ** 17n
The current implementation 10n * 17n results in 170 wei which is extremely low. It should be 10n ** 17n (0.1 ETH) to match the production deployments and other test files. The ** operator was mistakenly replaced with * during the BigNumber to BigInt conversion.
🔗 Analysis chain
Verify the feeForJuror calculation
While the change to native BigInt syntax is good, the feeForJuror calculation appears incorrect:
- Current:
10n * 17n= 170 wei (extremely low) - Expected: Should this be
10n * 10n ** 17nto represent 1e18 wei?
Let's verify the previous implementation:
Let me analyze the implementations of feeForJuror across the codebase:
- Production deployments:
00-home-chain-arbitration.ts:ETH(0.1)(0.1 ETH)00-home-chain-arbitration-neo.ts:ETH(0.1)(0.1 ETH)00-home-chain-arbitration-university.ts:ETH(0.1)(0.1 ETH)00-home-chain-arbitration-ruler.ts:10n ** 17n(0.1 ETH)
- Test/simulation files:
test/arbitration/draw.ts:10n * 17n(same as the reviewed code)scripts/simulations/tasks.ts:10n * 17n(same as the reviewed code)
Let me verify what ETH() function does:
Let me try a different approach to find the ETH helper:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for previous feeForJuror implementations
rg -A 2 "feeForJuror.*=.*" --type ts
Length of output: 3257
Script:
#!/bin/bash
# Search for ETH function definition
rg "function ETH" --type ts -A 2
Length of output: 34
Script:
#!/bin/bash
# Search for ETH definition or import
rg "ETH.*=|import.*ETH" --type ts -A 2
Length of output: 8348
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 16.x | ||
| node-version: 18.x |
There was a problem hiding this comment.
💡 Codebase verification
Node.js version upgrade needs adjustment to maintain compatibility
The codebase has explicit Node.js version constraints in multiple package.json files requiring version >=16.0.0 or >=16.13.0. While Node.js 18.x satisfies these constraints, it's recommended to update these version requirements to align with the workflow's Node.js 18.x specification.
- Update Node.js engine requirements in package.json files to
>=18.0.0for consistency with the CI environment - Consider adding an upper bound to prevent future compatibility issues (e.g.,
>=18.0.0 <19.0.0)
🔗 Analysis chain
Node.js version upgrade is appropriate.
The upgrade from Node.js 16.x to 18.x is necessary as v16 has reached end-of-life.
Let's verify Node.js compatibility in the project:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any Node.js version constraints in package.json files
echo "Checking Node.js version constraints in package.json files:"
fd package.json --exec jq -r 'select(.engines != null) | .engines.node'
Length of output: 230
🧰 Tools
🪛 yamllint
[error] 45-45: trailing spaces
(trailing-spaces)
| @@ -11,7 +12,7 @@ const foreignGatewayArtifactByChain = new Map<ForeignChains, string>([ | |||
| [ForeignChains.GNOSIS_CHIADO, "ForeignGatewayOnGnosis"], | |||
| ]); | |||
|
|
|||
| const ONE_GWEI = parseUnits("1", "gwei"); | |||
| const ONE_GWEI = BigNumber.from(parseUnits("1", "gwei")); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant BigNumber.from() wrapper
parseUnits already returns a BigNumber, making the BigNumber.from() wrapper redundant. This could potentially cause issues with type conversions.
-const ONE_GWEI = BigNumber.from(parseUnits("1", "gwei"));
+const ONE_GWEI = parseUnits("1", "gwei");📝 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.
| const ONE_GWEI = BigNumber.from(parseUnits("1", "gwei")); | |
| const ONE_GWEI = parseUnits("1", "gwei"); |
| @@ -22,7 +23,7 @@ | |||
| const chainId = Number(await getChainId()); | |||
| console.log("deploying to chainId %s with deployer %s", chainId, deployer); | |||
|
|
|||
| const foreignGatewayArtifact = foreignGatewayArtifactByChain.get(chainId) ?? ethers.constants.AddressZero; | |||
| const foreignGatewayArtifact = foreignGatewayArtifactByChain.get(chainId) ?? ethers.ZeroAddress; | |||
There was a problem hiding this comment.
Replace zero address fallback with explicit error handling
Using a zero address as a fallback when the foreign gateway artifact is not found could lead to silent failures. Consider throwing an error instead to make deployment failures more explicit.
- const foreignGatewayArtifact = foreignGatewayArtifactByChain.get(chainId) ?? ethers.ZeroAddress;
+ const foreignGatewayArtifact = foreignGatewayArtifactByChain.get(chainId);
+ if (!foreignGatewayArtifact) {
+ throw new Error(`No foreign gateway artifact configured for chain ID ${chainId}`);
+ }📝 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.
| const foreignGatewayArtifact = foreignGatewayArtifactByChain.get(chainId) ?? ethers.ZeroAddress; | |
| const foreignGatewayArtifact = foreignGatewayArtifactByChain.get(chainId); | |
| if (!foreignGatewayArtifact) { | |
| throw new Error(`No foreign gateway artifact configured for chain ID ${chainId}`); | |
| } |
chore(web): remove-testnet-banner
|
Summary by CodeRabbit
compressionLevelsetting and disabled global caching.3.7.0to4.5.1..yarnrc.ymlconfiguration file for better plugin management and settings organization.