Feature: Dynamic protocol fee #16
Conversation
WalkthroughProxy constructor drops the stored protocolFeeBps parameter and calls implementation initialize with three args. UniversalBridgeV1 moves protocolFeeBps out of storage into each TransactionRequest (kept alongside expirationTimestamp), updates EIP‑712 types, event signature, and fee distribution to use per-request protocolFeeBps. Tests updated to match new structures and proxy constructor. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant B as UniversalBridgeV1
participant T as Token/ETH
participant R as ProtocolFeeRecipient
participant D as DeveloperRecipient
rect rgb(240,248,255)
note right of U: Prepare EIP‑712 TransactionRequest\nincludes expirationTimestamp and protocolFeeBps (per-request)
U->>B: initiateTransaction(req, sig, ...)
B->>B: validate signature & req.protocolFeeBps <= MAX
B->>T: collect token/ETH amount
B->>B: compute protocolFee = amount * req.protocolFeeBps / 10_000
B->>B: compute developerFee = amount * req.developerFeeBps / 10_000
B->>R: transfer protocolFee
B->>D: transfer developerFee
B->>U: emit TransactionInitiated(..., protocolFee, ..., developerFee, ...)
end
sequenceDiagram
autonumber
participant P as UniversalBridgeProxy
participant I as UniversalBridgeV1 (impl)
Note over P,I: Deployment / Initialization
P->>I: delegatecall initialize(owner, operator, protocolFeeRecipient)
I->>I: set owner/operator/feeRecipient (no stored protocolFeeBps)
I-->>P: init result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
gregfromstl
force-pushed
the
feat/dynamic-protocol-fee
branch
2 times, most recently
from
db233dd to
127a7b9
Compare
gregfromstl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/UniversalBridgeV1.t.sol (1)
88-93: Fix EIP-712 type string: replace expirationTimestamp with protocolFeeBps.Tests are signing with
req.protocolFeeBpsbut the type string still namesexpirationTimestamp. Update to avoid mismatches with client tooling and to reflect the on-chain type.Apply:
- typehashTransactionRequest = keccak256( - "TransactionRequest(bytes32 transactionId,address tokenAddress,uint256 tokenAmount,address forwardAddress,address spenderAddress,uint256 expirationTimestamp,address developerFeeRecipient,uint256 developerFeeBps,bytes callData,bytes extraData)" - ); + typehashTransactionRequest = keccak256( + "TransactionRequest(bytes32 transactionId,address tokenAddress,uint256 tokenAmount,address forwardAddress,address spenderAddress,uint256 protocolFeeBps,address developerFeeRecipient,uint256 developerFeeBps,bytes callData,bytes extraData)" + );
🧹 Nitpick comments (5)
src/UniversalBridgeProxy.sol (2)
13-13: Constructor param removal looks good; confirm zero-operator semantics.Dropping protocolFeeBps from construction aligns with per-request fees. Please confirm that passing a zero
_operatoris intentional (owner can grant later via OwnableRoles). If not, add a non-zero check to fail fast.
30-35: Safer initializer encoding: prefer encodeWithSelector or abi.encodeCall.Using a string signature risks typos. Switch to a selector-based encoding to get compile-time checks.
Apply:
- bytes memory data = abi.encodeWithSignature( - "initialize(address,address,address)", - _owner, - _operator, - _protocolFeeRecipient - ); + // Minimal interface to obtain the selector without importing full impl + interface IInit { function initialize(address,address,address) external; } + bytes memory data = abi.encodeWithSelector( + IInit.initialize.selector, + _owner, + _operator, + _protocolFeeRecipient + );src/UniversalBridgeV1.sol (1)
203-209: Native direct transfer: consider requiring exact msg.value.When
req.tokenAddressis native andreq.callData.length == 0(direct pay), overpaying sends extra ETH to the receiver with no refund path. If that’s undesirable, enforce equality.Apply:
- if (_isNativeToken(req.tokenAddress)) { + if (_isNativeToken(req.tokenAddress)) { uint256 sendValue = msg.value - protocolFee - developerFee; // subtract total fee amount from sent value - - if (sendValue < req.tokenAmount) { - revert UniversalBridgeMismatchedValue(req.tokenAmount, sendValue); - } + if (req.callData.length == 0) { + if (sendValue != req.tokenAmount) { + revert UniversalBridgeMismatchedValue(req.tokenAmount, sendValue); + } + } else { + if (sendValue < req.tokenAmount) { + revert UniversalBridgeMismatchedValue(req.tokenAmount, sendValue); + } + } _call(req.forwardAddress, sendValue, req.callData); // calldata empty for direct transfertest/UniversalBridgeV1.t.sol (2)
484-514: Mismatched native value test is precise.Good negative test; will still pass after enforcing equality in direct native path (if you adopt that refactor, update expected actual accordingly).
1-1: Add a test for out-of-bounds developerFeeBps.If you adopt the bounds check, add a revert test to lock behavior.
Example addition:
function test_revert_developerFeeBps_overMax() public { UniversalBridgeV1.TransactionRequest memory req; req.transactionId = keccak256("dev fee bps"); req.tokenAddress = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE); req.tokenAmount = 1 ether; req.forwardAddress = payable(address(receiver)); req.spenderAddress = payable(address(0)); req.developerFeeRecipient = developer; req.developerFeeBps = 10_001; req.protocolFeeBps = protocolFeeBps; bytes memory _signature = _prepareAndSignData(6, req); vm.prank(sender); vm.expectRevert(UniversalBridgeV1.UniversalBridgeInvalidFeeBps.selector); bridge.initiateTransaction{ value: 1 ether + expectedProtocolFee }(req, _signature); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/UniversalBridgeProxy.sol(2 hunks)src/UniversalBridgeV1.sol(12 hunks)test/UniversalBridgeV1.t.sol(15 hunks)
🔇 Additional comments (10)
src/UniversalBridgeV1.sol (6)
103-107: Init changes OK; verify zero-operator behavior.Granting roles to a zero
_operatoris a no-op but allowed. Confirm this is intended; otherwise, add a non-zero check.
131-133: Protocol fee recipient setter simplification LGTM.Scope reduced to recipient only; storage removal of BPS is consistent with per-request fees.
143-145: Getter API change LGTM.Returning only the recipient matches the new model.
250-255: Event and fee distribution updates LGTM.
- Added
protocolFeetoTransactionInitiatedand correct ordering._distributeFeesnow consumesprotocolFeeBpsper request and splits correctly for native/ERC20.Also applies to: 72-82, 314-341
348-354: Zero-address guard for fee recipient LGTM.Consistent with initialize path; avoids accidental burn.
362-364: Pure accessor for storage pointer LGTM.Correct pattern for ERC-7201 namespaced storage.
test/UniversalBridgeV1.t.sol (4)
23-28: Event signature update LGTM.The
protocolFeeparameter positioning matches the contract event.
74-77: Proxy construction update LGTM.Four-arg constructor aligns with the proxy’s new initialize signature.
138-138: Per-requestprotocolFeeBpsusage LGTM.Consistent assignments across scenarios and signing.
Also applies to: 177-177, 221-221, 263-263, 309-309, 354-354, 395-395, 439-439, 499-499, 533-533, 640-640, 693-693
455-456: Event expectation update LGTM.
expectedProtocolFeeasserted in the correct position.
gregfromstl
force-pushed
the
feat/dynamic-protocol-fee
branch
from
127a7b9 to
aaac8b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/UniversalBridgeV1.t.sol (1)
88-96: EIP-712 typehash string still references expirationTimestamp — update to protocolFeeBps.The struct hash uses
req.protocolFeeBpsbelow, but the type string here namesexpirationTimestamp. Field names change the typehash; this will invalidate signatures if the contract usesprotocolFeeBps.Replace with:
- typehashTransactionRequest = keccak256( - "TransactionRequest(bytes32 transactionId,address tokenAddress,uint256 tokenAmount,address forwardAddress,address spenderAddress,uint256 expirationTimestamp,address developerFeeRecipient,uint256 developerFeeBps,bytes callData,bytes extraData)" - ); + typehashTransactionRequest = keccak256( + "TransactionRequest(bytes32 transactionId,address tokenAddress,uint256 tokenAmount,address forwardAddress,address spenderAddress,uint256 protocolFeeBps,address developerFeeRecipient,uint256 developerFeeBps,bytes callData,bytes extraData)" + );Also verify the contract’s declared type string matches this.
🧹 Nitpick comments (2)
src/UniversalBridgeProxy.sol (1)
13-24: Consider zero-address validation for operator and fee recipient.If zero values are not meaningful in your protocol, add explicit checks for
_operatorand_protocolFeeRecipientto fail-fast at deploy time.Apply:
constructor(address _implementation, address _owner, address _operator, address payable _protocolFeeRecipient) { if (_implementation == address(0)) { revert ImplementationZeroAddress(); } if (_implementation.code.length == 0) { revert ImplementationDoesNotExist(); } if (_owner == address(0)) { revert OwnerZeroAddress(); } + if (_operator == address(0)) { + revert("OperatorZeroAddress()"); + } + if (_protocolFeeRecipient == address(0)) { + revert("ProtocolFeeRecipientZeroAddress()"); + }test/UniversalBridgeV1.t.sol (1)
66-73: Add boundary tests for BPS limits and rounding.Consider adding tests for:
protocolFeeBps = 0anddeveloperFeeBps = 0- Sum of BPS near/at 10_000 (100%) and rejection > 10_000 if applicable
- Rounding with small amounts (values < 1 ether) for integer division edge cases
I can draft these test cases if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/UniversalBridgeProxy.sol(2 hunks)src/UniversalBridgeV1.sol(13 hunks)test/UniversalBridgeV1.t.sol(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/UniversalBridgeV1.sol
🔇 Additional comments (6)
src/UniversalBridgeProxy.sol (2)
13-21: Constructor updates look good; existence check is a solid safety add.Dropping protocolFeeBps from the ctor and adding the
code.lengthcheck reduces mis-deploy risk and keeps config in the implementation. No functional concerns here.
30-36: Initializer signature and one-time init guard verifiedThe
UniversalBridgeV1contract definesfunction initialize(address _owner, address _operator, address payable _protocolFeeRecipient) external initializer { … }which corresponds to the
initialize(address,address,address)signature used in the proxy’s delegatecall, and theinitializermodifier (from OpenZeppelin’sInitializable) enforces a one-time init guard. No further changes required.test/UniversalBridgeV1.t.sol (4)
131-144: EIP-712 struct encoding updated to include protocolFeeBps — correct.Switching from expirationTimestamp to
protocolFeeBpsin the encoded struct aligns with per-request fee computation.
176-179: Per-request protocolFeeBps usage across tests looks consistent.Assigning
req.protocolFeeBps = protocolFeeBps;in each scenario keeps signing and runtime behavior aligned.Also applies to: 220-222, 262-264, 307-309, 353-355, 393-395, 438-439
450-456: Event assertion updated for protocolFee — good alignment.
TransactionInitiatednow assertsexpectedProtocolFeein the new position. Looks consistent with the event definition.
484-514: Ignore fee-semantic concerns; the test’s(sendValue, sendValue – 1)matches the contract’s revert of(req.tokenAmount, sendValue), so no change needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/UniversalBridgeV1.sol (1)
318-349: Guard against burning ETH when developerFeeRecipient is zero.For native token flows, sending ETH to address(0) silently burns funds. Revert when developerFeeBps > 0 and developerFeeRecipient == address(0).
function _distributeFees( address tokenAddress, uint256 tokenAmount, address developerFeeRecipient, uint256 developerFeeBps, uint256 protocolFeeBps ) private returns (uint256, uint256) { + if (developerFeeBps != 0 && developerFeeRecipient == address(0)) { + revert UniversalBridgeZeroAddress(); + }
♻️ Duplicate comments (1)
src/UniversalBridgeV1.sol (1)
184-187: Protocol fee cap check — LGTM. Clarify developer fee policy.Cap for protocol fee is enforced; developerFeeBps remains uncapped by design. Add NatSpec/docs stating that developer fees may exceed 100% and are entirely dictated by the signed request to avoid integrator confusion.
Would you like a short NatSpec patch + a test asserting developerFeeBps > 10_000 works as intended?
🧹 Nitpick comments (7)
src/UniversalBridgeV1.sol (4)
171-171: Move “processed” flag after prechecks to avoid wasted SSTORE on reverting paths.Setting the flag before pause / restriction / fee-cap checks burns gas on reverts. Mark it only after all preconditions pass, just before fee distribution.
- // mark the pay request as processed - _universalBridgeStorage().processed[req.transactionId] = true; - if (_universalBridgeStorage().isPaused) { revert UniversalBridgePaused(); } @@ } + // All preconditions passed: mark processed just before side-effects. + _universalBridgeStorage().processed[req.transactionId] = true; + // verify amount if (req.tokenAmount == 0) {Also applies to: 173-187
105-109: Operator zero-address: footgun.Granting roles to address(0) is a common misconfig that bricks initiation (no signer can pass hasAllRoles). Either block zero or explicitly document it.
function initialize(address _owner, address _operator, address payable _protocolFeeRecipient) external initializer { _initializeOwner(_owner); - _grantRoles(_operator, _OPERATOR_ROLE); + if (_operator == address(0)) revert UniversalBridgeZeroAddress(); + _grantRoles(_operator, _OPERATOR_ROLE); _setProtocolFeeInfo(_protocolFeeRecipient); }
133-136: API naming nit: “FeeInfo” now only contains a recipient.Optional: rename to setProtocolFeeRecipient/getProtocolFeeRecipient for clarity. Keep aliases if you need backward compatibility.
Also applies to: 145-147
356-362: Emit event when protocol fee recipient changes.Improves observability for indexers and off-chain services.
- function _setProtocolFeeInfo(address payable feeRecipient) internal { + event ProtocolFeeRecipientUpdated(address indexed previous, address indexed next); + + function _setProtocolFeeInfo(address payable feeRecipient) internal { if (feeRecipient == address(0)) { revert UniversalBridgeZeroAddress(); } - - _universalBridgeStorage().protocolFeeRecipient = feeRecipient; + address prev = _universalBridgeStorage().protocolFeeRecipient; + _universalBridgeStorage().protocolFeeRecipient = feeRecipient; + emit ProtocolFeeRecipientUpdated(prev, feeRecipient); }test/UniversalBridgeV1.t.sol (3)
171-180: Good coverage of protocolFeeBps across paths. Add a negative test for cap enforcement.Please add a test that sets protocolFeeBps to MAX_PROTOCOL_FEE_BPS + 1 and expects UniversalBridgeInvalidFeeBps().
function test_revert_protocolFeeBps_exceeds_cap() public { UniversalBridgeV1.TransactionRequest memory req; req.transactionId = keccak256("cap"); req.expirationTimestamp = 1000; req.tokenAddress = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE); req.tokenAmount = 1 ether; req.developerFeeRecipient = developer; req.developerFeeBps = 0; req.protocolFeeBps = 301; // 300 is max bytes memory sig = _prepareAndSignData(6, req); vm.prank(sender); vm.expectRevert(UniversalBridgeV1.UniversalBridgeInvalidFeeBps.selector); bridge.initiateTransaction{ value: 1 ether }(req, sig); }Also applies to: 223-225, 266-268, 312-314, 359-361, 400-402, 446-447, 509-510, 544-544, 655-655, 709-709
66-73: Sanity tests to document fee policies.
- Add a test where developerFeeBps > 10_000 to assert it’s allowed and collected.
- Add a replay test to ensure the second call with the same transactionId reverts with UniversalBridgeTransactionAlreadyProcessed().
I can draft these tests if helpful.
312-314: If you adopt the zero developer recipient guard, add a test.Ensure native-token flows revert when developerFeeBps > 0 and developerFeeRecipient == address(0).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/UniversalBridgeV1.sol(13 hunks)test/UniversalBridgeV1.t.sol(16 hunks)
🔇 Additional comments (7)
src/UniversalBridgeV1.sol (3)
73-83: Event shape update — LGTM.Single protocolFee amount emitted; positions and values are consistent with the new logic.
Also applies to: 248-257
370-372: Storage accessor as pure — LGTM.Standard ERC‑7201 pattern; no state read occurs.
64-67: EIP‑712 struct/typehash alignment — verifiedMatching type string found in src/UniversalBridgeV1.sol (struct at line 50; typehash at lines 64–66; usage at 296) and in test/UniversalBridgeV1.t.sol (line 90). No stale type strings detected.
test/UniversalBridgeV1.t.sol (4)
18-28: Updated event signature — LGTM.Matches the on-chain event (single protocolFee amount).
76-76: Proxy constructor update — LGTM.Four-arg ctor aligns with dropped protocolFeeBps from initialization.
88-92: Typehash string update — LGTM.Includes protocolFeeBps after expirationTimestamp, consistent with the contract.
139-144: Signature payload update — LGTM.req.protocolFeeBps included in the signed data; ordering matches the type string.
Summary by CodeRabbit
New Features
Refactor
Tests
Bug Fixes