feat: Add processSettlement function to Gateway contract#66
feat: Add processSettlement function to Gateway contract#66onahprosper wants to merge 6 commits intomainfrom
Conversation
- Implemented processSettlement function in Gateway contract to handle order settlements, including sender fees and protocol fees. - Added ProcessSettlement event to IGateway interface and emitted it during settlement. - Updated Gateway contract to check for sender fees and protocol fees during token transfers. - Enhanced order struct to store additional settlement details. - Created comprehensive tests for processSettlement functionality, covering various scenarios including fee deductions. - Updated relevant interfaces and mock contracts to support new functionality.
…tency - Renamed event `OrderSettled` to `SettleOut` and updated its parameters accordingly. - Renamed event `ProcessSettlement` to `SettleIn` and adjusted its parameters. - Updated function names from `settle` to `settleOut` and from `processSettlement` to `settleIn` to reflect the new event names. - Modified the corresponding test cases to align with the new event and function names. - Updated event references in the utility manager for consistency.
…and state management
…ion with ECDSA signature verification
- Created JSON artifacts for IGateway and ProviderBatchCallAndSponsor contracts, including ABI and bytecode. - Added debug information for both contracts. - Ensured proper formatting and structure for compatibility with Hardhat.
WalkthroughThis PR updates numerous build artifacts, adds OpenZeppelin utility artifacts, introduces ProviderBatchCallAndSponsor, restructures Gateway settlement API (settle → settleOut; adds settleIn and getAggregator), updates IGateway accordingly, and adds/updates tests and an npm script. Changes
Sequence Diagram(s)sequenceDiagram
participant LP as LiquidityProvider
participant Gateway
participant Token as ERC20
participant Treasury
participant Recipient
LP->>Gateway: settleIn(orderId, token, amount, senderFeeRecipient, senderFee, recipient, rate, messageHash)
Gateway->>Token: transferFrom(LP, Gateway, amount)
alt senderFee > 0
Gateway->>Token: transfer(senderFeeRecipient, senderFee)
end
opt protocolFee > 0
Gateway->>Token: transfer(Treasury, protocolFee)
end
Gateway->>Token: transfer(Recipient, amount - senderFee - protocolFee)
Gateway-->>LP: returns true / emits SettleIn(...)
sequenceDiagram
participant Aggregator
participant Gateway
participant LP as LiquidityProvider
Aggregator->>Gateway: settleOut(splitOrderId, orderId, liquidityProvider, settlePercent)
Gateway-->>Aggregator: emit SettleOut(...)
Gateway-->>Aggregator: returns true
sequenceDiagram
participant Aggregator
participant PBS as ProviderBatchCallAndSponsor
participant Target as TargetContract
Aggregator->>PBS: execute(calls, signature)
PBS->>PBS: verify signature (ECDSA, nonce, packed calls)
loop per call
PBS->>Target: call(to, value, data)
PBS-->>Aggregator: emit CallExecuted(...)
end
PBS-->>Aggregator: emit BatchExecuted(nonce, calls)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 21
♻️ Duplicate comments (4)
artifacts/@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol/Ownable2StepUpgradeable.dbg.json (1)
3-3: Same buildInfo pointer update as others; see prior verification scriptThis mirrors the metadata update noted earlier. Ensure the build-info reference resolves.
artifacts/@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol/ContextUpgradeable.dbg.json (1)
3-3: Build-info metadata-only update; confirm pointer is validSame note as earlier: verify the referenced build-info JSON exists and is used consistently across artifacts.
artifacts/@openzeppelin/contracts/token/ERC20/ERC20.sol/ERC20.dbg.json (1)
3-3: Build-info pointer refresh; no ABI/interface changePure metadata update. Validate the pointer via the earlier script.
artifacts/@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol/AddressUpgradeable.dbg.json (1)
3-3: Build-info pointer update; validate consistencySame as others; ensure the pointer resolves and no stale build-info remains.
🧹 Nitpick comments (2)
contracts/ProviderBatchCallAndSponsore.sol (1)
72-78: Remove or properly document the non-functional execute functionHaving a function that always reverts with "Not implemented" is confusing for users. Either remove this function entirely or provide clearer documentation about why it exists but is not implemented.
Consider removing this function entirely if it's not intended to be used, or update the comment to clearly explain why this overload exists but is disabled.
contracts/Gateway.sol (1)
249-249: Fix inconsistent formattingRemove unnecessary tabs/spacing for consistent code formatting.
- if(_senderFee != 0) { + if(_senderFee != 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (34)
artifacts/@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol/Ownable2StepUpgradeable.dbg.json(1 hunks)artifacts/@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol/OwnableUpgradeable.dbg.json(1 hunks)artifacts/@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol/Initializable.dbg.json(1 hunks)artifacts/@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol/PausableUpgradeable.dbg.json(1 hunks)artifacts/@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol/AddressUpgradeable.dbg.json(1 hunks)artifacts/@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol/ContextUpgradeable.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/token/ERC20/ERC20.sol/ERC20.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/token/ERC20/IERC20.sol/IERC20.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol/IERC20Metadata.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/utils/Context.sol/Context.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/utils/Strings.sol/Strings.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/utils/Strings.sol/Strings.json(1 hunks)artifacts/@openzeppelin/contracts/utils/cryptography/ECDSA.sol/ECDSA.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/utils/cryptography/ECDSA.sol/ECDSA.json(1 hunks)artifacts/@openzeppelin/contracts/utils/math/Math.sol/Math.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/utils/math/Math.sol/Math.json(1 hunks)artifacts/@openzeppelin/contracts/utils/math/SignedMath.sol/SignedMath.dbg.json(1 hunks)artifacts/@openzeppelin/contracts/utils/math/SignedMath.sol/SignedMath.json(1 hunks)artifacts/contracts/Gateway.sol/Gateway.dbg.json(1 hunks)artifacts/contracts/GatewaySettingManager.sol/GatewaySettingManager.dbg.json(1 hunks)artifacts/contracts/ProviderBatchCallAndSponsore.sol/IGateway.dbg.json(1 hunks)artifacts/contracts/ProviderBatchCallAndSponsore.sol/IGateway.json(1 hunks)artifacts/contracts/ProviderBatchCallAndSponsore.sol/ProviderBatchCallAndSponsor.dbg.json(1 hunks)artifacts/contracts/ProviderBatchCallAndSponsore.sol/ProviderBatchCallAndSponsor.json(1 hunks)artifacts/contracts/interfaces/IGateway.sol/IGateway.dbg.json(1 hunks)artifacts/contracts/interfaces/IGateway.sol/IGateway.json(4 hunks)artifacts/contracts/mocks/MockUSDC.sol/MockUSDT.dbg.json(1 hunks)contracts/Gateway.sol(4 hunks)contracts/ProviderBatchCallAndSponsore.sol(1 hunks)contracts/interfaces/IGateway.sol(1 hunks)package.json(1 hunks)test/gateway/gateway.processorder.test.js(1 hunks)test/gateway/gateway.settleOrder.test.js(2 hunks)test/utils/utils.manager.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/gateway/gateway.processorder.test.js (2)
test/utils/utils.manager.js (3)
Events(28-38)ZERO_AMOUNT(4-4)FEE_BPS(7-7)test/fixtures/gateway.js (1)
gatewayFixture(16-16)
test/gateway/gateway.settleOrder.test.js (1)
test/utils/utils.manager.js (1)
Events(28-38)
🔇 Additional comments (25)
artifacts/@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol/Initializable.dbg.json (1)
3-3: LGTM: buildInfo reference updated.Artifact metadata only; no runtime or ABI changes.
artifacts/@openzeppelin/contracts/utils/math/SignedMath.sol/SignedMath.dbg.json (1)
1-4: New dbg artifact added — consistent with repo-wide buildInfo.Matches the new build-info hash; no concerns.
artifacts/@openzeppelin/contracts/utils/math/SignedMath.sol/SignedMath.json (1)
1-10: New artifact added for SignedMath.Empty ABI for a pure library is expected; bytecode/link refs look standard. No issues.
artifacts/@openzeppelin/contracts/utils/Context.sol/Context.dbg.json (1)
3-3: LGTM: buildInfo pointer switched to new hash.Metadata-only change.
artifacts/@openzeppelin/contracts/utils/Strings.sol/Strings.dbg.json (1)
1-4: New dbg artifact for Strings — OK.Consistent buildInfo reference; no functional changes implied.
artifacts/contracts/GatewaySettingManager.sol/GatewaySettingManager.dbg.json (1)
3-3: LGTM: debug metadata updated.No ABI/runtime changes; pointer aligns with repo-wide update.
artifacts/@openzeppelin/contracts/token/ERC20/IERC20.sol/IERC20.dbg.json (1)
3-3: LGTM: buildInfo updated.Interface-only artifact; safe metadata change.
artifacts/contracts/ProviderBatchCallAndSponsore.sol/IGateway.dbg.json (1)
3-3: Confirm buildInfo resolution at this depthRelative buildInfo uses "../../build-info/...". Given its shallower directory depth vs OZ artifacts, this looks correct. Still, please run the provided verification script to confirm no broken dbg.json pointers.
artifacts/@openzeppelin/contracts/utils/math/Math.sol/Math.dbg.json (1)
1-4: New OZ Math artifact addedLooks good. ABI-less library artifact is expected. No action needed beyond build-info verification.
artifacts/@openzeppelin/contracts/utils/cryptography/ECDSA.sol/ECDSA.json (1)
1-10: New OZ ECDSA artifact (library) — ABI empty as expectedLibrary artifact with empty ABI is correct. Bytecode/deployedBytecode present. No issues.
artifacts/@openzeppelin/contracts/utils/math/Math.sol/Math.json (1)
1-10: LGTM: OZ Math artifact added (ABI empty is expected for library).Metadata-only addition; no runtime impact.
artifacts/contracts/interfaces/IGateway.sol/IGateway.dbg.json (1)
3-3: Verify IGateway interface events/functions align with PR objectivesThe interface
contracts/interfaces/IGateway.solnow exposes separate on- and off-ramp methods and events (settleIn/SettleInandsettleOut/SettleOut), but the PR description and older artifacts reference a singleProcessSettlement(orOrderSettled) event. Please confirm and reconcile:• In
contracts/interfaces/IGateway.sol, the public API currently includes:
– event SettleOut(bytes32 splitOrderId, bytes32 indexed orderId, address indexed liquidityProvider, uint96 settlePercent)
– event SettleIn(bytes32 indexed orderId, uint256 indexed amount, address indexed recipient, address token, address senderFee, uint96 rate, string messageHash)
– functions settleOut and settleIn• In
artifacts/contracts/interfaces/IGateway.sol/IGateway.json, the ABI listsSettleIn,SettleOut,settleIn, andsettleOut—noProcessSettlementorOrderSettled.• The legacy build artifact
build/contracts/IGateway.jsonstill containsOrderSettled.Action items:
- Confirm whether the intended design is one consolidated
processSettlementAPI or two separate in/out calls.- Rename or remove events/functions in
IGateway.sol(and tests) to match the final API.- Regenerate all artifacts (ABI, debug JSON under
artifacts/andbuild/) so they reflect the updated event/function names.- Update any references in tests or documentation to the chosen event(s).
artifacts/@openzeppelin/contracts/utils/cryptography/ECDSA.sol/ECDSA.dbg.json (1)
1-4: LGTM: ECDSA dbg artifact added (metadata only).artifacts/@openzeppelin/contracts/utils/Strings.sol/Strings.json (1)
1-10: LGTM: OZ Strings artifact added (ABI empty is expected for library).No issues noted.
artifacts/@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol/IERC20Metadata.dbg.json (1)
3-3: LGTM: dbg buildInfo pointer updated.Repository-wide metadata sync; no functional change.
artifacts/contracts/mocks/MockUSDC.sol/MockUSDT.dbg.json (1)
3-3: Build artifact metadata update looks good.This is an expected change when contracts are recompiled. The buildInfo reference update is consistent with other artifact updates in this PR.
test/utils/utils.manager.js (1)
31-32: Event names correctly updated to match contract interface.The replacement of
OrderSettledwithSettleOutandSettleInproperly reflects the bidirectional settlement functionality introduced in the Gateway contract.artifacts/contracts/ProviderBatchCallAndSponsore.sol/IGateway.json (1)
1-24: Interface artifact structure looks correct.The IGateway interface artifact properly defines the
getAggregator()function with correct ABI structure. Empty bytecode is expected for interface artifacts.test/gateway/gateway.settleOrder.test.js (2)
200-203: Test correctly updated to use new settleOut function and SettleOut event.The test properly calls the renamed
settleOutfunction and verifies theSettleOutevent emission with the correct parameters.
308-311: Duplicate order settlement test correctly uses new API.The test for settling an already fulfilled order properly uses the updated
settleOutfunction andSettleOutevent.contracts/interfaces/IGateway.sol (4)
57-65: SettleIn event structure looks good.The new
SettleInevent properly captures all necessary information for onramp order processing with appropriate indexed parameters for efficient filtering.
171-180: Well-documented settleIn function for onramp processing.The new
settleInfunction has comprehensive documentation and a clear parameter structure that aligns with the processSettlement objectives mentioned in the PR summary.
140-145: settleOut function properly renamed with clear documentation.The renamed
settleOutfunction maintains backward compatibility with the parameter structure while providing clearer semantics for outbound settlements.
212-212: getAggregator function properly defined.The new
getAggregatorview function provides necessary access to the aggregator address, supporting the new settlement architecture.artifacts/contracts/interfaces/IGateway.sol/IGateway.json (1)
222-234: New view: getAggregator() — LGTMRead-only accessor returning an address. No issues.
| { | ||
| "_format": "hh-sol-dbg-1", | ||
| "buildInfo": "../../../../build-info/b8a88c4b130e7a569c3029307ef5f126.json" | ||
| "buildInfo": "../../../../build-info/5631a6977c9d28c95713f02c5a51ee48.json" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify buildInfo target and repo-wide pointer consistency
Metadata-only change. Please ensure the referenced build-info exists and that all dbg.json files now consistently point to it.
Run this to validate dbg.json pointers and existence of the new build-info:
Additionally, per PR objectives, confirm IGateway artifacts expose the new event/function:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show unique buildInfo targets referenced by all dbg.json files
echo "Unique buildInfo references:"
fd -a '\.dbg\.json$' | xargs -I{} sh -c 'jq -r ".buildInfo" "{}" 2>/dev/null || true' | sort | uniq -c
# 2) Verify the new build-info file actually exists in artifacts/build-info
TARGET_HASH="5631a6977c9d28c95713f02c5a51ee48"
echo -e "\nChecking build-info file existence:"
fd -a "$TARGET_HASH.json" artifacts/build-info || { echo "Missing build-info: $TARGET_HASH.json"; exit 1; }
# 3) Detect any dbg.json still pointing to removed/old build-info files
echo -e "\nAny dbg.json referencing non-existent build-info?"
missing=0
while IFS= read -r f; do
bi=$(jq -r '.buildInfo // empty' "$f" 2>/dev/null || true)
[ -z "$bi" ] && continue
# Normalize to repo-root path
realpath=$(python - <<PY
import os,sys
f=sys.argv[1]; bi=sys.argv[2]
rp=os.path.normpath(os.path.join(os.path.dirname(f), bi))
print(rp)
PY "$f" "$bi")
if [ ! -f "$realpath" ]; then
echo "Broken: $f -> $bi (resolved: $realpath)"
missing=1
fi
done < <(fd -a '\.dbg\.json$')
exit $missingLength of output: 1223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking IGateway artifact for ProcessSettlement and processSettlement:"
fd -a 'IGateway.json' artifacts | while read -r f; do
echo "File: $f"
jq '[.abi[] | select(.type=="event") | .name] | any(.=="ProcessSettlement")' "$f"
jq '[.abi[] | select(.type=="function") | .name] | any(.=="processSettlement")' "$f"
doneLength of output: 1280
Update IGateway ABI to include new event and function
The build-info pointers in all .dbg.json files consistently reference the same hash (5631a6977c9d28c95713f02c5a51ee48.json), and that file exists under artifacts/build-info/. No broken references were found.
However, neither of your IGateway artifacts exposes the new ProcessSettlement event or the processSettlement function:
• artifacts/contracts/ProviderBatchCallAndSponsore.sol/IGateway.json
• artifacts/contracts/interfaces/IGateway.sol/IGateway.json
Please update the IGateway interface (and re-generate its artifact) so that its ABI includes:
event ProcessSettlement(...)function processSettlement(...)
This will fulfill the PR objective of exposing the new event and function in the IGateway artifact.
🤖 Prompt for AI Agents
In
artifacts/@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol/OwnableUpgradeable.dbg.json
around line 3, the review notes that the IGateway ABI in the project artifacts
is missing the new ProcessSettlement event and processSettlement function;
update the IGateway interface source (contracts/interfaces/IGateway.sol) to add
the exact event declaration (event ProcessSettlement(...);) and function
signature (function processSettlement(...);) matching the implementation, then
recompile/regenerate the artifacts so that both artifacts
artifacts/contracts/ProviderBatchCallAndSponsore.sol/IGateway.json and
artifacts/contracts/interfaces/IGateway.sol/IGateway.json include the new
entries in their ABI; finally run your project compiler (e.g., hardhat/forge) to
produce updated build-info and artifact JSONs and commit those regenerated
artifact files.
| { | ||
| "_format": "hh-sol-dbg-1", | ||
| "buildInfo": "../../../../build-info/b8a88c4b130e7a569c3029307ef5f126.json" | ||
| "buildInfo": "../../../../build-info/5631a6977c9d28c95713f02c5a51ee48.json" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Metadata-only buildInfo pointer update — verify consistency across artifacts.
No behavioral impact. Please verify all dbg.json now reference the new build-info and that the file exists.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
NEW_HASH=5631a6977c9d28c95713f02c5a51ee48
OLD_HASH=b8a88c4b130e7a569c3029307ef5f126
# Ensure new build-info file exists
fd -a "${NEW_HASH}\.json" build-info || { echo "ERROR: build-info/${NEW_HASH}.json not found"; exit 1; }
# Show all dbg.json buildInfo refs
echo "BuildInfo references in artifacts:"
rg -n '"buildInfo": ' -g 'artifacts/**.dbg.json' | sed -n 's/^\(.*\):\([0-9]*\):\s*"buildInfo":\s*"\([^"]*\)".*/\1:\2 -> \3/p' | sort
# Ensure no dbg.json still points to old hash
if rg -n "$OLD_HASH" -g 'artifacts/**.dbg.json' >/dev/null; then
echo "WARNING: Found dbg.json still referencing old build-info hash:"
rg -n "$OLD_HASH" -g 'artifacts/**.dbg.json'
exit 2
fi
# Verify each referenced buildInfo path exists
FAIL=0
while IFS= read -r ref; do
file="$(echo "$ref" | sed -n 's/.*-> \(.*\)$/\1/p')"
[ -f "$file" ] || { echo "ERROR: Missing buildInfo target: $file (from $ref)"; FAIL=1; }
done < <(rg -n '"buildInfo": ' -g 'artifacts/**.dbg.json' | sed -n 's/^\(.*\):\([0-9]*\):\s*"buildInfo":\s*"\([^"]*\)".*/\1:\2 -> \3/p')
exit $FAILLength of output: 415
#!/bin/bash
set -euo pipefail
echo "1) Locating all directories named build-info in the repo..."
fd -t d build-info .
echo
echo "2) Checking for the new build-info JSON in each found directory..."
for dir in $(fd -t d build-info .); do
target="$dir/5631a6977c9d28c95713f02c5a51ee48.json"
if [ -f "$target" ]; then
echo "✔ FOUND: $target"
else
echo "✖ MISSING: $target"
fi
done
echo
echo "3) Extracting all dbg.json → buildInfo paths and verifying file existence:"
rg -oP '"buildInfo":\s*"\K[^"]+' -g 'artifacts/**/*.dbg.json' | sort -u | while read -r relpath; do
resolve relative to each dbg.json location
dbgfile=$(rg -l ""buildInfo":.$relpath" -g 'artifacts/**/.dbg.json')
base=$(dirname "$dbgfile")
full="$base/$relpath"
if [ -f "$full" ]; then
echo "✔ $dbgfile → $relpath exists"
else
echo "✖ $dbgfile → $relpath is missing"
missing=1
fi
done
exit "${missing:-0}"
#!/bin/bash
set -euo pipefail
echo "1) Locating all build-info directories in the repo:"
fd -t d 'build-info' .
echo
echo "2) Searching for the new build-info JSON anywhere:"
fd -e json '5631a6977c9d28c95713f02c5a51ee48.json' .
echo
echo "3) Verifying each dbg.json → buildInfo pointer:"
fail=0
for dbg in $(rg -l '"buildInfo":' -g 'artifacts/**/*.dbg.json'); do
info=$(jq -r .buildInfo "$dbg")
target="$(dirname "$dbg")/$info"
if [ -f "$target" ]; then
echo "✔ $dbg → $info exists"
else
echo "✖ $dbg → $info missing"
fail=1
fi
if grep -q 'b8a88c4b130e7a569c3029307ef5f126' "$dbg"; then
echo "✖ Old hash still referenced in $dbg"
fail=1
fi
done
exit $fail
---
#!/bin/bash
set -euo pipefail
echo "1) Locate all ‘build-info’ directories:"
find . -type d -name 'build-info'
echo
echo "2) Find the new build-info JSON anywhere in the repo:"
find . -type f -name '5631a6977c9d28c95713f02c5a51ee48.json'
echo
echo "3) Validate each dbg.json → buildInfo pointer:"
missing=0
for dbg in $(find artifacts -type f -name '*.dbg.json'); do
ptr=$(jq -r .buildInfo "$dbg")
target="$(dirname "$dbg")/$ptr"
if [ -f "$target" ]; then
echo "✔ $dbg → $ptr"
else
echo "✖ $dbg → $ptr is missing"
missing=1
fi
if grep -q 'b8a88c4b130e7a569c3029307ef5f126' "$dbg"; then
echo "✖ $dbg still references the old hash"
missing=1
fi
done
exit $missing
---
</details>
**Broken buildInfo pointers – missing build-info directory and JSON file**
I ran a verification script and discovered that the expected `build-info/5631a6977c9d28c95713f02c5a51ee48.json` doesn’t exist at the indicated path, and all your `*.dbg.json` artifacts still point to a non-existent file (and may still reference the old hash). Please:
- Confirm where the `build-info` directory actually lives in the repo (it may have been moved or renamed).
- Ensure `5631a6977c9d28c95713f02c5a51ee48.json` is committed under that directory.
- Update each `artifacts/**/*.dbg.json`’s `"buildInfo"` pointer to the correct relative path.
- Remove any lingering references to the old hash `b8a88c4b130e7a569c3029307ef5f126`.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In
artifacts/@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol/PausableUpgradeable.dbg.json
around line 3, the "buildInfo" pointer references a missing path
build-info/5631a6977c9d28c95713f02c5a51ee48.json; locate the actual build-info
directory in the repo (search for build-info or the correct json file), commit
the missing 5631a6977c9d28c95713f02c5a51ee48.json into that directory, update
the "buildInfo" value in this dbg.json (and all artifacts/**/*.dbg.json files)
to the correct relative path to that committed file, and run a repo-wide replace
to remove any lingering references to the old hash
b8a88c4b130e7a569c3029307ef5f126 so all artifact pointers and hashes are
consistent.
</details>
<!-- fingerprinting:phantom:triton:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
| { | ||
| "_format": "hh-sol-dbg-1", | ||
| "buildInfo": "../../build-info/b92a18bd36be0e82f59e420b9f0d4c50.json" | ||
| "buildInfo": "../../build-info/5631a6977c9d28c95713f02c5a51ee48.json" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Dbg buildInfo updated; please ensure settlement functions are safe (reentrancy, ERC20 compliance).
Given settlement involves token transfers and fee handling, confirm:
- Use of OpenZeppelin SafeERC20 for transfers/transferFrom.
- nonReentrant guard around settleIn/settleOut/processSettlement.
- Valid fee recipients (non-zero) and bounds checks to prevent over-deduction.
🏁 Script executed:
#!/bin/bash
# Quick static checks on Gateway source for safety patterns
set -euo pipefail
fd Gateway.sol | while read -r f; do
echo "==> Inspecting $f"
rg -n 'processSettlement|settleIn|settleOut|nonReentrant|SafeERC20|safeTransferFrom|safeTransfer|protocolFee|senderFee' "$f" -n || true
done
echo "Check tests cover fee deductions and edge cases..."
fd -t f 'test' | rg -n 'processorder|settleOrder|protocol fee|sender fee|SafeERC20|nonReentrant' || trueLength of output: 9247
Add SafeERC20 transfers and nonReentrant guards to Gateway settlement functions
The current Gateway.sol uses raw IERC20.transfer/transferFrom and has no reentrancy protection around settleIn, settleOut, or processSettlement. To harden against token‐level failures and reentrancy:
• In contracts/Gateway.sol
– Import and use OpenZeppelin’s SafeERC20 library:
• Replace
solidity IERC20(_token).transferFrom(msg.sender, address(this), _amount + _senderFee); IERC20(token).transfer(treasuryAddress, protocolFee);
with
solidity using SafeERC20 for IERC20; IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount + _senderFee); IERC20(token).safeTransfer(treasuryAddress, protocolFee);
– Inherit ReentrancyGuard and add nonReentrant to each of:
• function settleIn(...)
• function settleOut(...)
• function processSettlement(...) (if defined externally)
– Add require(treasuryAddress != address(0), "InvalidTreasury") before any protocol fee transfer.
• Fee‐recipient validation (already present):
if (_senderFee != 0) {
require(_senderFeeRecipient != address(0), "InvalidSenderFeeRecipient");
}• Tests
– Extend test/gateway/*.test.js to cover:
• Reentrancy attempts on settleIn/settleOut
• Failure scenarios when ERC20.transfer would return false (using a mock token)
These changes will ensure safe token operations and guard against reentrancy.
🤖 Prompt for AI Agents
In artifacts/contracts/Gateway.sol/Gateway.dbg.json around line 3 (refers to
contracts/Gateway.sol), the Gateway uses raw IERC20.transfer/transferFrom and
lacks reentrancy guards; replace raw token calls with OpenZeppelin SafeERC20 by
importing SafeERC20, adding "using SafeERC20 for IERC20" and replacing
transfer/transferFrom with safeTransfer/safeTransferFrom, inherit
ReentrancyGuard and add the nonReentrant modifier to settleIn, settleOut (and
processSettlement if externally callable), add a require(treasuryAddress !=
address(0), "InvalidTreasury") check immediately before any protocol fee
transfer, keep the existing sender fee recipient validation, and add tests that
simulate token transfer failures and reentrancy attempts against
settleIn/settleOut.
| "indexed": true, | ||
| "internalType": "address", | ||
| "name": "sender", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "internalType": "uint256", | ||
| "name": "amount", | ||
| "type": "uint256" | ||
| } | ||
| ], | ||
| "name": "SenderFeeTransferred", | ||
| "type": "event" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Event design: include token and reconsider indexing amount
In multi-token systems, this event should include the token address for observability. Also, indexing the amount is rarely useful and adds gas; prefer indexing address-like fields.
Apply this diff (update Solidity and re-generate the artifact; diff illustrates the ABI change):
"inputs": [
{
"indexed": true,
"internalType": "address",
"name": "sender",
"type": "address"
},
+ {
+ "indexed": true,
+ "internalType": "address",
+ "name": "token",
+ "type": "address"
+ },
{
- "indexed": true,
+ "indexed": false,
"internalType": "uint256",
"name": "amount",
"type": "uint256"
}
],
"name": "SenderFeeTransferred",
"type": "event"📝 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.
| "indexed": true, | |
| "internalType": "address", | |
| "name": "sender", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": true, | |
| "internalType": "uint256", | |
| "name": "amount", | |
| "type": "uint256" | |
| } | |
| ], | |
| "name": "SenderFeeTransferred", | |
| "type": "event" | |
| }, | |
| "inputs": [ | |
| { | |
| "indexed": true, | |
| "internalType": "address", | |
| "name": "sender", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": true, | |
| "internalType": "address", | |
| "name": "token", | |
| "type": "address" | |
| }, | |
| { | |
| "indexed": false, | |
| "internalType": "uint256", | |
| "name": "amount", | |
| "type": "uint256" | |
| } | |
| ], | |
| "name": "SenderFeeTransferred", | |
| "type": "event" | |
| }, |
🤖 Prompt for AI Agents
In artifacts/contracts/interfaces/IGateway.sol/IGateway.json around lines 78 to
92, the SenderFeeTransferred event ABI lacks a token address field and currently
indexes the amount (a uint256), which is unnecessary and costly; update the
IGateway Solidity interface to add an address token parameter to the
SenderFeeTransferred event and remove the indexed modifier from amount (keep
sender and token indexed as address-like fields), then recompile/contracts and
re-generate the JSON artifact so the ABI reflects the new event signature.
| { | ||
| "indexed": true, | ||
| "internalType": "uint256", | ||
| "name": "amount", | ||
| "type": "uint256" | ||
| }, | ||
| { | ||
| "indexed": true, | ||
| "internalType": "address", | ||
| "name": "liquidityProvider", | ||
| "name": "recipient", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "internalType": "address", | ||
| "name": "token", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "internalType": "address", | ||
| "name": "senderFee", | ||
| "type": "address" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "internalType": "uint96", | ||
| "name": "settlePercent", | ||
| "name": "rate", | ||
| "type": "uint96" | ||
| }, | ||
| { | ||
| "indexed": false, | ||
| "internalType": "string", | ||
| "name": "messageHash", | ||
| "type": "string" | ||
| } | ||
| ], | ||
| "name": "OrderSettled", | ||
| "name": "SettleIn", | ||
| "type": "event" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
SettleIn event: field/type/indexing issues (misnamed address, indexing amount, string hash)
- Field name/type mismatch: “senderFee” is an address but reads like an amount. Rename to senderFeeRecipient (address) to avoid confusion and to match the function param naming.
- Indexing: amount is indexed; token is not. Consider indexing token and leaving amount non-indexed for better queryability and lower gas.
- messageHash as string is expensive; bytes32 is typical for hashes.
Proposed ABI adjustments (reflect in Solidity, then regenerate):
"inputs": [
{
"indexed": true,
"internalType": "bytes32",
"name": "orderId",
"type": "bytes32"
},
{
- "indexed": true,
+ "indexed": false,
"internalType": "uint256",
"name": "amount",
"type": "uint256"
},
{
"indexed": true,
"internalType": "address",
"name": "recipient",
"type": "address"
},
{
- "indexed": false,
+ "indexed": true,
"internalType": "address",
"name": "token",
"type": "address"
},
{
"indexed": false,
"internalType": "address",
- "name": "senderFee",
+ "name": "senderFeeRecipient",
"type": "address"
},
{
"indexed": false,
"internalType": "uint96",
"name": "rate",
"type": "uint96"
},
{
"indexed": false,
- "internalType": "string",
+ "internalType": "bytes32",
"name": "messageHash",
- "type": "string"
+ "type": "bytes32"
}
],
"name": "SettleIn",
"type": "event"Additionally, PR summary mentions a ProcessSettlement event. There’s no such event in this ABI. Was it renamed to SettleIn/SettleOut? If ProcessSettlement is expected, please add it or update the PR narrative.
🏁 Script executed:
#!/bin/bash
# Verify references to a ProcessSettlement event/function across the repo.
rg -n --no-ignore -S 'processSettlement|ProcessSettlement'Length of output: 277359
Align SettleIn event ABI & docs
- Rename parameter for clarity: use senderFeeRecipient (address) instead of senderFee.
- Rebalance indexed topics (max 3): index orderId, recipient, token; unindex amount.
- Optimize hash type: use bytes32 for messageHash instead of expensive string.
- Fix outdated NatSpec: in contracts/Gateway.sol` replace
- /** @dev See {processSettlement-IGateway}. */ + /** @dev See {settleIn-IGateway}. */
Apply these changes in IGateway.sol and regenerate artifacts/JSON.
Example diff in interface IGateway.sol:
event SettleIn(
- bytes32 indexed orderId,
- uint256 indexed amount,
- address indexed recipient,
- address token,
- address senderFee,
- uint96 rate,
- string messageHash
-);
+ bytes32 indexed orderId,
+ address indexed recipient,
+ address indexed token,
+ address senderFeeRecipient,
+ uint96 rate,
+ bytes32 messageHash
+);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In artifacts/contracts/interfaces/IGateway.sol/IGateway.json around lines 102 to
141, the SettleIn event ABI is out of sync with the intended interface and docs:
rename the address parameter senderFee to senderFeeRecipient, change indexed
topics so only orderId, recipient and token are indexed and amount is not
indexed (max 3 indexed), change messageHash type from string to bytes32, update
the corresponding NatSpec in contracts/Gateway.sol to match the new parameter
name and types, then apply the same signature changes in IGateway.sol source and
finally regenerate the JSON artifact so the ABI reflects these edits.
| "ownership": "npx hardhat test test/paycrest/paycrest.ownable.test.js", | ||
| "settleOrder": "npx hardhat test test/paycrest/paycrest.settleOrder.test.js", | ||
| "createOrder": "npx hardhat test test/paycrest/paycrest.createorder.test.js", | ||
| "processOrder": "npx hardhat test test/gateway/gateway.processorder.test.js", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New script “processOrder” added; ensure CI/docs updated and test path exists.
Good addition. Please:
- Add this script to CI if it’s meant to run separately.
- Ensure the referenced test file exists and aligns with the finalized API (processSettlement vs settleIn/settleOut).
🏁 Script executed:
#!/bin/bash
# Validate test file presence and quick peek at its references
set -euo pipefail
test -f test/gateway/gateway.processorder.test.js && echo "Found test/gateway/gateway.processorder.test.js" || (echo "Missing test file"; exit 1)
rg -n 'processSettlement|settleIn|settleOut|sender fee|protocol fee' test/gateway/gateway.processorder.test.js || trueLength of output: 807
🏁 Script executed:
#!/bin/bash
# Check if the new “processOrder” npm script is referenced in CI workflows or documentation
set -euo pipefail
# Search in GitHub Actions workflows
if rg -n 'processOrder' .github/workflows; then
echo "Found references to processOrder in CI workflows."
else
echo "No references to processOrder in CI workflows."
fi
# Search in README or docs folder
if rg -n 'processOrder' README.md docs; then
echo "Found references to processOrder in documentation."
else
echo "No references to processOrder in documentation."
fiLength of output: 347
Add processOrder script to CI workflows and documentation
The test file test/gateway/gateway.processorder.test.js exists and correctly uses .settleIn(...). To complete this change, please:
- Update your CI workflows (e.g. under
.github/workflows/) to invoke the newprocessOrderscript if it’s intended to run as part of your pipeline. - Add or update your documentation (README.md or a docs/ folder) to include instructions for running the
processOrdernpm script.
🤖 Prompt for AI Agents
In package.json around line 12, you added a "processOrder" npm script but CI and
docs weren't updated; add a step to your CI workflows (e.g.,
.github/workflows/*.yml) to run the script as part of the test/job that executes
unit tests—ensure the job installs dependencies (npm ci) and invokes npm run
processOrder (or npx hardhat test test/gateway/gateway.processorder.test.js) at
the appropriate point in the test matrix; also update README.md (or docs/) with
a short instruction showing how to run the new script (npm run processOrder),
what it tests (gateway.processorder), and any prerequisites (e.g., environment
variables, Hardhat config) so contributors and CI callers know how to execute
it.
| // charge 0.1% as protocol fee | ||
| const protocolFeePercent = BigNumber.from(100); | ||
|
|
||
| this.protocolFee = ethers.utils.parseEther("27000") |
There was a problem hiding this comment.
Fix incorrect comment about protocol fee percentage
The comment states "charge 0.1% as protocol fee" but the protocolFeePercent is set to 100 BPS, which equals 1%, not 0.1%. The calculated protocolFee of 27000 ETH (0.1% of 27000000) suggests the comment should be "charge 1% as protocol fee".
- // charge 0.1% as protocol fee
+ // charge 1% as protocol fee
const protocolFeePercent = BigNumber.from(100);📝 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.
| // charge 0.1% as protocol fee | |
| const protocolFeePercent = BigNumber.from(100); | |
| this.protocolFee = ethers.utils.parseEther("27000") | |
| // charge 1% as protocol fee | |
| const protocolFeePercent = BigNumber.from(100); | |
| this.protocolFee = ethers.utils.parseEther("27000") |
🤖 Prompt for AI Agents
In test/gateway/gateway.processorder.test.js around lines 39 to 42 the inline
comment incorrectly says "charge 0.1% as protocol fee" while protocolFeePercent
is set to 100 BPS (1%) and the protocolFee calculation matches 1%; update the
comment to "charge 1% as protocol fee" so it accurately reflects the code and
calculation.
| expect(this.seller).to.eq(this.recipient.address); | ||
| expect(this.token).to.eq(mockUSDT.address); | ||
| expect(this.senderRecipient).to.eq(this.sender.address); | ||
| expect(this.senderFee).to.eq(this.senderFee); |
There was a problem hiding this comment.
Fix self-comparison in assertion
The assertion compares this.senderFee to itself. It should compare the returned senderFee from the contract to the expected value (0 in this case).
- expect(this.senderFee).to.eq(this.senderFee);
+ expect(this.senderFee).to.eq(ethers.utils.parseEther("0"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(this.senderFee).to.eq(this.senderFee); | |
| - expect(this.senderFee).to.eq(this.senderFee); | |
| + expect(this.senderFee).to.eq(ethers.utils.parseEther("0")); |
🤖 Prompt for AI Agents
In test/gateway/gateway.processorder.test.js around line 167, the test currently
asserts this.senderFee equals itself which is a no-op; replace the right-hand
operand with the expected sender fee (0) or the appropriate expected variable
(e.g., expectedSenderFee) so the assertion actually validates the contract
return value against the expected value.
| expect(await mockUSDT.balanceOf(this.recipient.address)).to.eq( | ||
| this.orderAmount.sub(((this.protocolFee)).sub(this.senderFee)) | ||
| ); |
There was a problem hiding this comment.
Fix incorrect balance calculation for recipient
The recipient balance calculation is incorrect. When senderFee is 0, the recipient should receive orderAmount - protocolFee. The current calculation orderAmount - (protocolFee - senderFee) would result in orderAmount - protocolFee when senderFee is 0, but the parentheses make it confusing and error-prone.
expect(await mockUSDT.balanceOf(this.recipient.address)).to.eq(
- this.orderAmount.sub(((this.protocolFee)).sub(this.senderFee))
+ this.orderAmount.sub(this.protocolFee)
);📝 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.
| expect(await mockUSDT.balanceOf(this.recipient.address)).to.eq( | |
| this.orderAmount.sub(((this.protocolFee)).sub(this.senderFee)) | |
| ); | |
| expect(await mockUSDT.balanceOf(this.recipient.address)).to.eq( | |
| this.orderAmount.sub(this.protocolFee) | |
| ); |
🤖 Prompt for AI Agents
In test/gateway/gateway.processorder.test.js around lines 177 to 179, the
recipient balance assertion uses a confusing expression that computes
orderAmount.sub((protocolFee).sub(senderFee)); replace it with a clear, explicit
calculation: use this.orderAmount.sub(this.protocolFee) for the recipient
expected balance (or if senderFee must also be deducted from the recipient, use
this.orderAmount.sub(this.protocolFee).sub(this.senderFee)); update the
assertion to use the correct, unambiguous subtraction.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/gateway/gateway.settleOrder.test.js (1)
316-336: Fix inconsistent API usage - still using old settle method.Lines 316, 334 still call the old
settle()method and lines 317, 335 still expect the oldOrderSettledevent, but the API was renamed tosettleOut()andSettleOut.Apply this diff to fix the inconsistent API usage:
expect( await gateway .connect(this.aggregator) - .settle(splitOrderId, orderId, this.liquidityProvider.address, splitOrderpercent) - ).to.emit(gateway, Events.Gateway.OrderSettled) + .settleOut(splitOrderId, orderId, this.liquidityProvider.address, splitOrderpercent) + ).to.emit(gateway, Events.Gateway.SettleOut) .withArgs(splitOrderId, orderId, this.liquidityProvider.address, splitOrderpercent);expect( await gateway .connect(this.aggregator) - .settle(splitOrderId, orderId, this.liquidityProvider2.address, splitOrderpercent) - ).to.emit(gateway, Events.Gateway.OrderSettled) + .settleOut(splitOrderId, orderId, this.liquidityProvider2.address, splitOrderpercent) + ).to.emit(gateway, Events.Gateway.SettleOut) .withArgs(splitOrderId, orderId, this.liquidityProvider2.address, splitOrderpercent);
♻️ Duplicate comments (1)
package.json (1)
12-12: Add processOrder script to CI workflows and documentationThe processOrder script was added but CI workflows and documentation still need updates as noted in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/Gateway.sol(4 hunks)package.json(1 hunks)test/gateway/gateway.settleOrder.test.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/gateway/gateway.settleOrder.test.js (2)
test/gateway/gateway.createorder.test.js (5)
orderId(112-112)orderId(213-213)orderId(288-288)orderId(363-363)orderId(437-437)test/utils/utils.manager.js (2)
MAX_BPS(6-6)Events(28-38)
🔇 Additional comments (5)
package.json (1)
9-11: LGTM on test path reorganization.The script path updates from
test/gateway/totest/paycrest/for ownership, settleOrder, and createOrder scripts appear to be part of a logical test reorganization.test/gateway/gateway.settleOrder.test.js (2)
201-204: LGTM on API rename to settleOut.The update from
settle()tosettleOut()andOrderSettledtoSettleOutevent is correctly implemented.
443-446: LGTM on API rename to settleOut.The update from
settle()tosettleOut()andOrderSettledtoSettleOutevent is correctly implemented.contracts/Gateway.sol (2)
147-200: LGTM on function rename and logic consistency.The rename from
settletosettleOutmaintains all the original logic while updating the event emission toSettleOut. The function correctly handles settlement percentage calculations, protocol fees, and state updates.
317-319: LGTM on getAggregator implementation.Simple and correct getter function for the aggregator address.
| /** @dev See {processSettlement-IGateway}. */ | ||
| function settleIn( | ||
| bytes32 _orderId, | ||
| address _token, | ||
| uint256 _amount, | ||
| address _senderFeeRecipient, | ||
| uint96 _senderFee, | ||
| address _recipient, | ||
| uint96 _rate, | ||
| string calldata _messageHash | ||
| ) external whenNotPaused returns (bool) { | ||
| require(!order[_orderId].isFulfilled, 'OrderNotFulfilled'); | ||
| require(_amount > MAX_BPS, 'AmountIsZero'); | ||
| _handler(_token, _amount, _recipient, _senderFeeRecipient, _senderFee); | ||
|
|
||
| IERC20(_token).transferFrom(msg.sender, address(this), _amount); | ||
|
|
||
| uint256 processedAmount = _amount; | ||
|
|
||
| if(_senderFee != 0) { | ||
| processedAmount -= _senderFee; | ||
|
|
||
| IERC20(_token).transfer( | ||
| _senderFeeRecipient, | ||
| _senderFee | ||
| ); | ||
|
|
||
| emit SenderFeeTransferred( | ||
| _senderFeeRecipient, | ||
| _senderFee | ||
| ); | ||
| } | ||
|
|
||
| uint256 protocolFee = (_amount * protocolFeePercent) / MAX_BPS; | ||
|
|
||
| if (protocolFee > 0) { | ||
| processedAmount -= protocolFee; | ||
| IERC20(_token).transfer(treasuryAddress, protocolFee); | ||
| } | ||
|
|
||
| IERC20(_token).transfer(_recipient, processedAmount); | ||
|
|
||
| // record the order state | ||
| order[_orderId].sender = _recipient; | ||
| order[_orderId].token = _token; | ||
| order[_orderId].senderFeeRecipient = _senderFeeRecipient; | ||
| order[_orderId].senderFee = _senderFee; | ||
| order[_orderId].protocolFee = protocolFee; | ||
| order[_orderId].isFulfilled = true; | ||
| order[_orderId].amount = processedAmount; | ||
|
|
||
| // emit settlement event | ||
| emit SettleIn( | ||
| _orderId, | ||
| _amount, | ||
| _recipient, | ||
| _token, | ||
| _senderFeeRecipient, | ||
| _rate, | ||
| _messageHash | ||
| ); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix validation logic and improve fee calculation in settleIn.
The new settleIn function has several issues:
-
Validation Logic: Lines 244-245 have the same issues flagged in past reviews - incorrect validation logic and inappropriate use of
MAX_BPS. -
Fee Calculation Logic: Lines 266, 252-253, 269 have problematic fee deduction order. The protocol fee is calculated on the full
_amountbut deducted fromprocessedAmountafter sender fees.
Apply these fixes:
- require(!order[_orderId].isFulfilled, 'OrderNotFulfilled');
- require(_amount > MAX_BPS, 'AmountIsZero');
+ require(!order[_orderId].isFulfilled, 'OrderAlreadyFulfilled');
+ require(_amount > 0, 'AmountIsZero'); uint256 processedAmount = _amount;
if(_senderFee != 0) {
processedAmount -= _senderFee;
IERC20(_token).transfer(
_senderFeeRecipient,
_senderFee
);
emit SenderFeeTransferred(
_senderFeeRecipient,
_senderFee
);
}
- uint256 protocolFee = (_amount * protocolFeePercent) / MAX_BPS;
+ uint256 protocolFee = (processedAmount * protocolFeePercent) / MAX_BPS;
if (protocolFee > 0) {
processedAmount -= protocolFee;
IERC20(_token).transfer(treasuryAddress, protocolFee);
}📝 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.
| /** @dev See {processSettlement-IGateway}. */ | |
| function settleIn( | |
| bytes32 _orderId, | |
| address _token, | |
| uint256 _amount, | |
| address _senderFeeRecipient, | |
| uint96 _senderFee, | |
| address _recipient, | |
| uint96 _rate, | |
| string calldata _messageHash | |
| ) external whenNotPaused returns (bool) { | |
| require(!order[_orderId].isFulfilled, 'OrderNotFulfilled'); | |
| require(_amount > MAX_BPS, 'AmountIsZero'); | |
| _handler(_token, _amount, _recipient, _senderFeeRecipient, _senderFee); | |
| IERC20(_token).transferFrom(msg.sender, address(this), _amount); | |
| uint256 processedAmount = _amount; | |
| if(_senderFee != 0) { | |
| processedAmount -= _senderFee; | |
| IERC20(_token).transfer( | |
| _senderFeeRecipient, | |
| _senderFee | |
| ); | |
| emit SenderFeeTransferred( | |
| _senderFeeRecipient, | |
| _senderFee | |
| ); | |
| } | |
| uint256 protocolFee = (_amount * protocolFeePercent) / MAX_BPS; | |
| if (protocolFee > 0) { | |
| processedAmount -= protocolFee; | |
| IERC20(_token).transfer(treasuryAddress, protocolFee); | |
| } | |
| IERC20(_token).transfer(_recipient, processedAmount); | |
| // record the order state | |
| order[_orderId].sender = _recipient; | |
| order[_orderId].token = _token; | |
| order[_orderId].senderFeeRecipient = _senderFeeRecipient; | |
| order[_orderId].senderFee = _senderFee; | |
| order[_orderId].protocolFee = protocolFee; | |
| order[_orderId].isFulfilled = true; | |
| order[_orderId].amount = processedAmount; | |
| // emit settlement event | |
| emit SettleIn( | |
| _orderId, | |
| _amount, | |
| _recipient, | |
| _token, | |
| _senderFeeRecipient, | |
| _rate, | |
| _messageHash | |
| ); | |
| return true; | |
| } | |
| /** @dev See {processSettlement-IGateway}. */ | |
| function settleIn( | |
| bytes32 _orderId, | |
| address _token, | |
| uint256 _amount, | |
| address _senderFeeRecipient, | |
| uint96 _senderFee, | |
| address _recipient, | |
| uint96 _rate, | |
| string calldata _messageHash | |
| ) external whenNotPaused returns (bool) { | |
| // Corrected validation logic and error messages | |
| require(!order[_orderId].isFulfilled, 'OrderAlreadyFulfilled'); | |
| require(_amount > 0, 'AmountIsZero'); | |
| _handler(_token, _amount, _recipient, _senderFeeRecipient, _senderFee); | |
| IERC20(_token).transferFrom(msg.sender, address(this), _amount); | |
| uint256 processedAmount = _amount; | |
| if (_senderFee != 0) { | |
| processedAmount -= _senderFee; | |
| IERC20(_token).transfer( | |
| _senderFeeRecipient, | |
| _senderFee | |
| ); | |
| emit SenderFeeTransferred( | |
| _senderFeeRecipient, | |
| _senderFee | |
| ); | |
| } | |
| // Calculate protocol fee on the net (post‐sender‐fee) amount | |
| uint256 protocolFee = (processedAmount * protocolFeePercent) / MAX_BPS; | |
| if (protocolFee > 0) { | |
| processedAmount -= protocolFee; | |
| IERC20(_token).transfer(treasuryAddress, protocolFee); | |
| } | |
| IERC20(_token).transfer(_recipient, processedAmount); | |
| // record the order state | |
| order[_orderId].sender = _recipient; | |
| order[_orderId].token = _token; | |
| order[_orderId].senderFeeRecipient = _senderFeeRecipient; | |
| order[_orderId].senderFee = _senderFee; | |
| order[_orderId].protocolFee = protocolFee; | |
| order[_orderId].isFulfilled = true; | |
| order[_orderId].amount = processedAmount; | |
| // emit settlement event | |
| emit SettleIn( | |
| _orderId, | |
| _amount, | |
| _recipient, | |
| _token, | |
| _senderFeeRecipient, | |
| _rate, | |
| _messageHash | |
| ); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In contracts/Gateway.sol around lines 233 to 296, the validation and fee math
are wrong: change the first require to assert the order is not already fulfilled
with a clear message (e.g. require(!order[_orderId].isFulfilled,
"OrderAlreadyFulfilled")) and replace the incorrect MAX_BPS check with a
zero-amount check (require(_amount > 0, "AmountIsZero")); then compute sender
fee first (if any) by subtracting _senderFee from processedAmount and
transferring it, and only after that calculate the protocol fee from the
remaining processedAmount (protocolFee = (processedAmount * protocolFeePercent)
/ MAX_BPS), subtract and transfer the protocol fee, ensure processedAmount is
reduced accordingly before transferring to _recipient, and finally record the
net processedAmount in the order struct and emit the event.
1 participant
Description
References
Testing
Checklist
mainBy submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores