Release to Beta #120
Release to Beta #120
Conversation
…elivery-deadline-and-dispute-deadline feat: buffer between delivery deadline and dispute deadline
Arbitrator devnet redeploy
feat(Escrow): add custom buyer option, contract-level only
✅ Deploy Preview for kleros-escrow-v2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis update introduces a new escrow contract variant, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebApp
participant BufferRules
participant EscrowContract
User->>WebApp: Initiate new transaction
WebApp->>BufferRules: pickBufferFor(creationUnixSec)
BufferRules-->>WebApp: bufferSec
WebApp->>EscrowContract: createTransaction(disputeDeadline = deliveryDeadline + bufferSec)
EscrowContract-->>WebApp: Transaction created
Note over WebApp: On transaction details page...
WebApp->>BufferRules: pickBufferFor(transactionTimestamp)
BufferRules-->>WebApp: bufferSec
WebApp->>User: Show BufferPeriodWarning if in buffer period
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
contracts/README.md (1)
136-136: Consider updating test fixture instructions for completeness.The test fixture section only shows running with
EscrowUniversaltag. Consider adding instructions for testing withEscrowCustomBuyeras well, or clarify if only one variant is needed for testing.**Shell 1: the node** ```bash yarn hardhat node --tags EscrowUniversal +# or +yarn hardhat node --tags EscrowCustomBuyer</blockquote></details> <details> <summary>contracts/scripts/changeArbitrator.ts (1)</summary><blockquote> `5-9`: **Remove unused parameters object.** The parameters object is defined but never used in the task. Consider removing it to reduce clutter. ```diff -const parameters = { - arbitrumSepoliaDevnet: {}, - arbitrumSepoliaTestnet: {}, - arbitrum: {}, -};web/src/pages/MyTransactions/TransactionDetails/index.tsx (1)
76-76: Consider potential edge case with buffer calculation.While the implementation is correct, consider adding validation to ensure the buffer period doesn't result in a negative delivery deadline.
const disputeDeadlineMs = deadline * 1000; const bufferSecNumber = transactionInfo?.bufferSec ?? pickBufferFor(timestamp); - const deliveryDeadlineMs = disputeDeadlineMs - bufferSecNumber * 1000; + const deliveryDeadlineMs = Math.max(0, disputeDeadlineMs - bufferSecNumber * 1000);web/src/utils/bufferRules.ts (1)
5-5: Consider adding a comment for buffer duration calculation.The magic number
604800could benefit from a comment showing the calculation for clarity.- { fromUnixSec: 1753315200, bufferSec: 7 * 24 * 60 * 60 }, // starts Jul 24 2025 00:00:00 UTC, buffer = 1 week (604800s) + { fromUnixSec: 1753315200, bufferSec: 7 * 24 * 60 * 60 }, // starts Jul 24 2025 00:00:00 UTC, buffer = 1 week (7*24*60*60 = 604800s)contracts/test/EscrowCustomBuyer.t.sol (1)
165-207: Consider adding tests for edge cases and error conditions.While the happy path tests are comprehensive, consider adding tests for:
- Transactions with insufficient balance
- Zero amount transactions
- Past deadlines
- Same address for buyer and seller
- Exceeding amount caps
Would you like me to help generate additional test cases for these edge conditions?
contracts/src/EscrowCustomBuyer.sol (1)
45-68: Function implementation is correct, but clarify the design choice.The logic is sound and follows the expected pattern for creating escrow transactions. However, the comment on line 60-61 highlights an important design choice that may need documentation.
Consider adding clearer documentation about why the caller (not the buyer) provides the funds in this variant, as this differs from typical escrow patterns where the buyer deposits funds.
📜 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 (22)
contracts/README.md(4 hunks)contracts/README.md.template(3 hunks)contracts/deploy/00-escrow-custom-buyer.ts(1 hunks)contracts/deploy/00-escrow-universal.ts(1 hunks)contracts/deploy/shared.ts(3 hunks)contracts/deploy/utils/getContracts.ts(2 hunks)contracts/deployments/arbitrum/USDC.json(1 hunks)contracts/deployments/arbitrum/USDCe.json(1 hunks)contracts/foundry.toml(1 hunks)contracts/hardhat.config.ts(1 hunks)contracts/package.json(1 hunks)contracts/scripts/changeArbitrator.ts(1 hunks)contracts/scripts/dotenv.sh(1 hunks)contracts/scripts/generateDeploymentsMarkdown.sh(1 hunks)contracts/scripts/setDisputeTemplate.ts(3 hunks)contracts/src/EscrowCustomBuyer.sol(1 hunks)contracts/test/EscrowCustomBuyer.t.sol(1 hunks)web/package.json(1 hunks)web/src/pages/MyTransactions/TransactionDetails/InfoCards/BufferPeriodWarning.tsx(1 hunks)web/src/pages/MyTransactions/TransactionDetails/index.tsx(4 hunks)web/src/pages/NewTransaction/NavigationButtons/DepositPaymentButton.tsx(4 hunks)web/src/utils/bufferRules.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
web/src/pages/MyTransactions/TransactionDetails/index.tsx (1)
web/src/utils/bufferRules.ts (1)
pickBufferFor(8-12)
contracts/deploy/00-escrow-universal.ts (1)
contracts/deploy/utils/index.ts (1)
isSkipped(30-36)
contracts/scripts/changeArbitrator.ts (1)
contracts/deploy/utils/getContracts.ts (1)
getContracts(22-30)
web/src/pages/NewTransaction/NavigationButtons/DepositPaymentButton.tsx (1)
web/src/utils/bufferRules.ts (1)
pickBufferFor(8-12)
contracts/deploy/00-escrow-custom-buyer.ts (1)
contracts/deploy/utils/index.ts (1)
isSkipped(30-36)
contracts/scripts/setDisputeTemplate.ts (1)
contracts/deploy/utils/getContracts.ts (1)
getContracts(22-30)
web/src/pages/MyTransactions/TransactionDetails/InfoCards/BufferPeriodWarning.tsx (2)
web/src/context/TransactionDetailsContext.tsx (1)
useTransactionDetailsContext(11-19)web/src/utils/formatTimeoutDuration.ts (1)
formatTimeoutDuration(1-14)
🪛 Shellcheck (0.10.0)
contracts/scripts/dotenv.sh
[warning] 13-13: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Redirect rules - kleros-escrow-v2
- GitHub Check: Redirect rules - kleros-escrow-v2
- GitHub Check: Header rules - kleros-escrow-v2
- GitHub Check: Header rules - kleros-escrow-v2
- GitHub Check: Pages changed - kleros-escrow-v2
- GitHub Check: Pages changed - kleros-escrow-v2
🔇 Additional comments (43)
contracts/deployments/arbitrum/USDCe.json (1)
1-3: Confirm the address is the canonical USDC.e token on Arbitrum
0xff970a61a04b1ca14834a43f5de4533ebddb5cc8is widely known as USDC.e, but hard-coding production addresses in source control can lead to costly mistakes if a wrong artefact is ever committed.
Please double-check the checksum and network, and consider adding an automated address validation step in the deployment pipeline.contracts/package.json (1)
85-88: Version bump to@kleros/kleros-v2-contracts^0.10.0 – verify compile/test pass
0.10.0contains several breaking Solidity changes (cf. changelog). Ensure:
- Typechain types regenerate without errors.
- All escrow contract imports (
Escrow,HomeArbitrableetc.) still resolve.- Gas snapshots / size limits remain within bounds.
If CI already covers those checks you can ignore this note.
web/package.json (1)
48-52: Keep web & contracts dependency ranges in lock-stepThe web app moved to
@kleros/kleros-v2-contracts@^0.10.0as a devDependency.
Double-check that vite/wagmi codegen still refers to the correct artefact paths after the version bump (module path changed from/artifacts/to/deployments/in 0.10).contracts/foundry.toml (1)
4-5:via_ir = true– good for size, but Foundry ≥ 0.6.6 requiredEnabling the IR pipeline usually reduces byte-code size but:
- Requires Solidity ≥ 0.8.22 and Foundry with
solcsupport for IR (0.6.6+).- Some Yul-based optimisations behave differently; rerun gas snapshots.
Looks good if CI passes.contracts/hardhat.config.ts (1)
15-18:changeArbitratorscript loaded – ensure task name collision is avoidedIncluding
require("./scripts/changeArbitrator")registers a Hardhat task at import-time. Please check:
- The script wraps its registration in
task("change-arbitrator", …)without colliding with existing task names.- It does not execute logic at import time (only task registration).
- The file path is correct when running
hardhatfrom the project root.If all good, 🍻.
contracts/scripts/generateDeploymentsMarkdown.sh (1)
11-12: LGTM - Consistent filtering of token artifactsThe addition of USDC and USDCe to the ignored artifacts list is appropriate for excluding standard token contracts from deployment documentation.
contracts/README.md.template (3)
80-82: LGTM - Clear separation of escrow variantsThe split into separate deployment commands for EscrowUniversal and EscrowCustomBuyer provides clear control over which contracts to deploy.
105-107: LGTM - Consistent testnet deployment instructionsThe testnet deployment commands properly mirror the local deployment structure.
120-121: Consider adding EscrowCustomBuyer tag for test fixturesThe test fixtures section only mentions EscrowUniversal tag. Should this also include EscrowCustomBuyer for comprehensive testing, or is there a specific reason to test only the universal variant?
contracts/deploy/00-escrow-universal.ts (1)
1-15: LGTM - Clean and focused deployment scriptThe script follows deployment best practices with proper imports, network compatibility checking, and clear tagging. The delegation to shared deployment logic promotes code reuse.
contracts/deploy/utils/getContracts.ts (2)
3-3: LGTM - Proper import additionThe EscrowCustomBuyer type is correctly added to the imports.
27-29: LGTM - Consistent contract instance handlingThe new contract instances follow the same pattern as existing contracts with proper typing and naming conventions.
contracts/scripts/dotenv.sh (1)
5-10: LGTM - Proper input validationGood error handling for missing arguments with appropriate error message to stderr.
contracts/README.md (4)
96-98: LGTM: Deployment commands updated correctly.The documentation now properly reflects the need to deploy each escrow variant separately using specific tags.
121-123: LGTM: Public testnet deployment instructions updated.The deployment commands are consistent with the local network instructions and properly specify the tags for each variant.
22-22: Confirm EscrowCustomBuyer contract is deployed at 0xA01e6B988aeDae1fD4a748D6bfBcB8A438601DeEThe RPC
eth_getCodequery returned no bytecode. Please verify on Arbitrum Sepolia Devnet (for example, via Arbiscan) that the address is correct and the contract is live.• File: contracts/README.md, line 22
25-25: Validate EscrowViewCustomBuyer Contract Deployment AddressThe RPC call to
eth_getCodeat0xe0892815E8958f0ad6Dab876995987c4F439954Dreturned no bytecode. Please confirm that this address is correct and that the contract is deployed on Arbitrum Sepolia Devnet by:
- Running locally:
curl -s -X POST https://sepolia.arbitrum.io/rpc \ -H "Content-Type: application/json" \ -d '{"jsonrpc":"2.0","method":"eth_getCode","params":["0xe0892815E8958f0ad6Dab876995987c4F439954D","latest"],"id":1}'- Verifying on the block explorer:
https://sepolia.arbiscan.io/address/0xe0892815E8958f0ad6Dab876995987c4F439954Dcontracts/scripts/changeArbitrator.ts (1)
12-12: No changes needed—both escrow variants supportchangeArbitrator.We confirmed via grep that
EscrowUniversaldeclares:function changeArbitrator(IArbitratorV2 _arbitrator) external onlyByGovernor { … }and that
EscrowCustomBuyerinherits this method (no override). Therefore, the Hardhat script’s call toescrow.changeArbitrator(...)works identically whether the deployed contract isEscrowUniversalorEscrowCustomBuyer.web/src/pages/MyTransactions/TransactionDetails/index.tsx (3)
75-77: LGTM: Buffer period calculation is well-implemented.The logic correctly:
- Converts deadline from seconds to milliseconds
- Uses fallback to
pickBufferForwhenbufferSecis not in transaction info- Properly calculates delivery deadline by subtracting buffer period
83-83: LGTM: BufferPeriodWarning component integration.The component is properly positioned and receives the necessary deadline props for buffer period warnings.
94-94: LGTM: PreviewCard updated to use delivery deadline.The change correctly passes the calculated delivery deadline instead of the original dispute deadline, maintaining the intended user experience.
contracts/deploy/00-escrow-custom-buyer.ts (1)
1-15: LGTM: Deployment script follows best practices.The script is well-structured with:
- Correct imports and types
- Proper use of shared deployment logic
- Appropriate skip condition based on supported networks
- Consistent tagging convention
web/src/utils/bufferRules.ts (3)
1-1: LGTM: Clear and appropriate type definition.The
BufferRuletype properly defines the structure with Unix timestamp and buffer duration in seconds.
8-12: LGTM: Buffer selection logic is correct.The
pickBufferForfunction correctly:
- Uses reduce to iterate through rules
- Selects buffer from the latest applicable rule (creationUnixSec >= fromUnixSec)
- Falls back to the first rule's buffer as initial value
3-6: Unix timestamp verification successfulThe timestamp
1753315200correctly corresponds to Jul 24 2025 00:00:00 UTC as intended—no changes needed.web/src/pages/NewTransaction/NavigationButtons/DepositPaymentButton.tsx (2)
64-71: LGTM! Proper implementation of buffer period logic.The timestamp calculations correctly handle the conversion to Unix seconds and maintain consistent units throughout. The buffer selection based on the current time aligns with the buffer rules implementation.
124-124: Correct usage of dispute deadline in contract calls.The contract simulation calls properly use
disputeDeadlineTimestampfor both native and ERC20 transactions, maintaining consistency with the new buffer period design.Also applies to: 144-144
contracts/scripts/setDisputeTemplate.ts (3)
100-106: Clean refactoring to support multiple escrow variants.The contract destructuring properly retrieves all necessary contracts, aligning with the updated
getContractsutility that now returns both universal and custom buyer escrow variants.
115-127: Well-structured helper function for dispute template configuration.The extracted
setDisputeTemplatefunction promotes code reuse and maintains type safety with the union type. Good error handling with transaction receipt logging.
128-133: Appropriate conditional handling of optional custom buyer variant.The code correctly handles the optional nature of the custom buyer escrow deployment, preventing errors on networks where it's not deployed.
web/src/pages/MyTransactions/TransactionDetails/InfoCards/BufferPeriodWarning.tsx (3)
20-24: Efficient timer management with proper cleanup.The effect correctly manages the interval timer, avoiding unnecessary updates when the transaction is already in dispute. Good use of cleanup function.
26-30: Correct buffer period detection and time calculation.The component properly identifies when the transaction is in the buffer period and calculates the remaining time with appropriate bounds checking.
34-42: Appropriate role-based messaging with correct action options.The component correctly identifies user roles with case-insensitive comparison and displays appropriate messages based on available actions for each role.
contracts/deploy/shared.ts (5)
6-14: Well-structured type definitions for deployment variants.The types clearly define the deployment structure with appropriate optionality for the custom buyer variant.
16-46: Excellent use of TypeScript's satisfies operator for type-safe configuration.The configuration structure clearly defines which escrow variants are deployed per network, with proper type checking at compile time.
48-51: Clean refactoring to support dynamic escrow variant deployment.The function properly parameterizes the deployment variant and uses the configuration to deploy the correct contracts.
Also applies to: 64-65, 79-80
82-98: Correct implementation of additional token caps.The code properly handles optional token deployments and uses the correct decimal places for each token type (6 for USDC variants, 18 for PNK).
108-108: Unable to locate historical gas usage for EscrowView deploymentI couldn’t find any logs or scripts showing actual gas used by the EscrowView deployment after reducing the gasLimit from 50 000 000 to 30 000 000. Please manually verify that 30 M is still sufficient by, for example:
- Running a forked-network dry run of the deployment (
npx hardhat run scripts/deploy/shared.ts --network <network>) and checkinggasUsed.- Adding a quick gas estimate in a test or script with
await deployments.estimateGas('EscrowView', { /* … */ })orethers.provider.estimateGas(…).Confirm that deployments on all target networks succeed without hitting the 30 M limit.
contracts/test/EscrowCustomBuyer.t.sol (3)
1-54: Comprehensive test setup with proper event definitions.The test contract is well-structured with all necessary imports, mock contracts, and test data properly initialized.
55-95: Thorough test initialization with all necessary preparations.The setUp function properly initializes mock contracts, funds test accounts, and configures the escrow contract with appropriate caps and approvals.
123-163: Comprehensive test coverage for native transactions with custom buyer.The test thoroughly validates event emission, transaction storage, and balance changes. Correctly verifies that the custom buyer address is stored while the actual payer's balance is reduced.
contracts/src/EscrowCustomBuyer.sol (1)
17-43: LGTM! Constructor implementation is correct.The constructor properly delegates all parameters to the parent
EscrowUniversalcontract and follows standard inheritance patterns.contracts/deployments/arbitrum/USDC.json (1)
1-1391: Official USDC address confirmedThe contract address
0xaf88d065e77c8cc2239327c5edb3a432268e5831matches the Circle-issued, Arbitrum-native USDC deployment (distinct from bridged USDC.e). No changes needed.
| import deployEscrow from "./shared"; | ||
|
|
||
| const deploy: DeployFunction = async (hre: HardhatRuntimeEnvironment) => { | ||
| await deployEscrow(hre, "customBuyer"); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the "customBuyer" parameter matches shared deployment logic.
Ensure the "customBuyer" parameter is correctly handled in the shared deployment function.
🏁 Script executed:
#!/bin/bash
# Description: Check shared deployment function handles customBuyer parameter
# Expected: Function should have logic for customBuyer variant
ast-grep --pattern 'function deployEscrow($_, $_) {
$$$
}'Length of output: 62
🏁 Script executed:
#!/bin/bash
# Locate the deployEscrow definition and its usages across the repo
rg -n "deployEscrow" .Length of output: 483
🏁 Script executed:
#!/bin/bash
# Display the implementation of deployEscrow in shared.ts
sed -n '1,200p' contracts/deploy/shared.tsLength of output: 3524
Ensure customBuyer is configured before deploying
The deployEscrow helper assumes config[network.name].escrowDeployments.customBuyer is defined. On networks like arbitrum, this entry is undefined, so running:
await deployEscrow(hre, "customBuyer");
will throw at runtime. Please address this by:
- In
contracts/deploy/shared.ts, adding a guard or explicit error whenescrowDeployments[escrowDeployment]is undefined before accessing.escrow/.view. - Or in
contracts/deploy/00-escrow-custom-buyer.ts, wrapping thedeployEscrow(hre, "customBuyer")call in a conditional that skips or falls back whencustomBuyerisn’t configured.
🤖 Prompt for AI Agents
In contracts/deploy/00-escrow-custom-buyer.ts at line 7, the call to
deployEscrow with "customBuyer" assumes the configuration exists, but on some
networks like arbitrum it is undefined causing runtime errors. Fix this by
adding a check before calling deployEscrow to verify that
config[network.name].escrowDeployments.customBuyer is defined; if not, skip the
deployment or provide a fallback. Alternatively, modify
contracts/deploy/shared.ts to add a guard or throw a clear error when
escrowDeployments[escrowDeployment] is undefined before accessing its
properties.
| task("change-arbitrator", "Changes the arbitrator").setAction(async (args, hre) => { | ||
| const { escrow, klerosCore } = await getContracts(hre); | ||
|
|
||
| console.log("changing Arbitrator..."); | ||
| const tx = await escrow.changeArbitrator(klerosCore.target); | ||
| await tx.wait().then((receipt) => { | ||
| console.log(`changeArbitrator() receipt: ${JSON.stringify(receipt)}`); | ||
| }); | ||
|
|
||
| console.log("changing DisputeTemplate..."); | ||
| await hre.run("set-dispute-template"); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for robustness.
The task should handle potential failures gracefully, especially for blockchain transactions.
task("change-arbitrator", "Changes the arbitrator").setAction(async (args, hre) => {
- const { escrow, klerosCore } = await getContracts(hre);
-
- console.log("changing Arbitrator...");
- const tx = await escrow.changeArbitrator(klerosCore.target);
- await tx.wait().then((receipt) => {
- console.log(`changeArbitrator() receipt: ${JSON.stringify(receipt)}`);
- });
-
- console.log("changing DisputeTemplate...");
- await hre.run("set-dispute-template");
+ try {
+ const { escrow, klerosCore } = await getContracts(hre);
+
+ console.log("changing Arbitrator...");
+ const tx = await escrow.changeArbitrator(klerosCore.target);
+ const receipt = await tx.wait();
+ console.log(`changeArbitrator() receipt: ${JSON.stringify(receipt)}`);
+
+ console.log("changing DisputeTemplate...");
+ await hre.run("set-dispute-template");
+ console.log("Successfully changed arbitrator and updated dispute templates");
+ } catch (error) {
+ console.error("Failed to change arbitrator:", error);
+ throw error;
+ }
});📝 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.
| task("change-arbitrator", "Changes the arbitrator").setAction(async (args, hre) => { | |
| const { escrow, klerosCore } = await getContracts(hre); | |
| console.log("changing Arbitrator..."); | |
| const tx = await escrow.changeArbitrator(klerosCore.target); | |
| await tx.wait().then((receipt) => { | |
| console.log(`changeArbitrator() receipt: ${JSON.stringify(receipt)}`); | |
| }); | |
| console.log("changing DisputeTemplate..."); | |
| await hre.run("set-dispute-template"); | |
| }); | |
| task("change-arbitrator", "Changes the arbitrator").setAction(async (args, hre) => { | |
| try { | |
| const { escrow, klerosCore } = await getContracts(hre); | |
| console.log("changing Arbitrator..."); | |
| const tx = await escrow.changeArbitrator(klerosCore.target); | |
| const receipt = await tx.wait(); | |
| console.log(`changeArbitrator() receipt: ${JSON.stringify(receipt)}`); | |
| console.log("changing DisputeTemplate..."); | |
| await hre.run("set-dispute-template"); | |
| console.log("Successfully changed arbitrator and updated dispute templates"); | |
| } catch (error) { | |
| console.error("Failed to change arbitrator:", error); | |
| throw error; | |
| } | |
| }); |
🤖 Prompt for AI Agents
In contracts/scripts/changeArbitrator.ts around lines 11 to 22, the task lacks
error handling for blockchain transactions, which can cause unhandled promise
rejections or silent failures. Wrap the asynchronous calls, especially the
escrow.changeArbitrator transaction and the hre.run("set-dispute-template")
call, in try-catch blocks to catch and log errors. This will ensure the task
handles failures gracefully and provides useful error messages.
| node -e " | ||
| require('dotenv').config({ path: '"$SCRIPT_DIR"/../.env' }) | ||
| console.log(process.env.$varKey) | ||
| " | sed 1d |
There was a problem hiding this comment.
Fix security vulnerability and quoting issue
The script has several issues:
- Security risk: The variable name
$varKeyis directly interpolated into Node.js code without validation, potentially allowing code injection - Quoting issue: Static analysis correctly identifies that
$varKeyneeds proper escaping within the Node.js string - Unclear
sed 1d: The purpose of removing the first line is unclear
Apply this fix to address the security and quoting issues:
-node -e "
- require('dotenv').config({ path: '"$SCRIPT_DIR"/../.env' })
- console.log(process.env.$varKey)
-" | sed 1d
+# Validate variable name (alphanumeric and underscore only)
+if [[ ! "$varKey" =~ ^[a-zA-Z_][a-zA-Z0-9_]*$ ]]; then
+ echo "Invalid variable name. Only alphanumeric characters and underscores allowed." >&2
+ exit 1
+fi
+
+node -e "
+ require('dotenv').config({ path: '"$SCRIPT_DIR"/../.env' })
+ console.log(process.env['$varKey'] || '')
+"This fix:
- Validates the variable name to prevent injection
- Uses bracket notation to safely access the environment variable
- Removes the unclear
sed 1d(please clarify if this filtering is actually needed)
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 13-13: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
🤖 Prompt for AI Agents
In contracts/scripts/dotenv.sh around lines 12 to 15, fix the security
vulnerability by validating the variable name $varKey before using it in the
Node.js code to prevent code injection. Replace direct interpolation with
bracket notation to safely access the environment variable in process.env. Also,
remove the unclear 'sed 1d' command unless there is a confirmed need to filter
out the first line. Ensure proper quoting and escaping of $varKey within the
Node.js string to avoid syntax errors.
| // Transfers token from sender wallet to contract. Note that in this version of the contract the amount is provided by the caller, not by the buyer. | ||
| if (!_token.safeTransferFrom(msg.sender, address(this), _amount)) revert TokenTransferFailed(); |
There was a problem hiding this comment.
Fix incorrect SafeERC20 usage - critical issue.
The safeTransferFrom function from SafeERC20 library doesn't return a boolean. It reverts on failure, making the boolean check incorrect and causing a compilation error.
Apply this fix:
- if (!_token.safeTransferFrom(msg.sender, address(this), _amount)) revert TokenTransferFailed();
+ _token.safeTransferFrom(msg.sender, address(this), _amount);The SafeERC20 library will automatically revert with its own error message if the transfer fails, so the TokenTransferFailed() revert is redundant.
📝 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.
| // Transfers token from sender wallet to contract. Note that in this version of the contract the amount is provided by the caller, not by the buyer. | |
| if (!_token.safeTransferFrom(msg.sender, address(this), _amount)) revert TokenTransferFailed(); | |
| // Transfers token from sender wallet to contract. Note that in this version of the contract the amount is provided by the caller, not by the buyer. | |
| - if (!_token.safeTransferFrom(msg.sender, address(this), _amount)) revert TokenTransferFailed(); | |
| + _token.safeTransferFrom(msg.sender, address(this), _amount); |
🤖 Prompt for AI Agents
In contracts/src/EscrowCustomBuyer.sol at lines 86-87, the usage of
safeTransferFrom incorrectly checks its boolean return value, but SafeERC20's
safeTransferFrom does not return a boolean and reverts on failure. Remove the if
statement and the boolean check, and call safeTransferFrom directly without
wrapping it in a conditional or revert, as the library will revert automatically
on failure.
PR-Codex overview
This PR introduces new features and updates for the
Escrowcontracts, including the addition ofEscrowCustomBuyer, support for new tokens, and improvements to deployment scripts and utility functions.Detailed summary
USDC.jsonandUSDCe.jsonto deployment scripts.@kleros/kleros-v2-contractsdependency version to^0.10.0.changeArbitratorscript.EscrowCustomBuyercontract for custom buyer transactions.setDisputeTemplateto support bothEscrowUniversalandEscrowCustomBuyer.BufferPeriodWarningcomponent for transaction details.README.mdwith new deployment commands.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests