Feature: oft adapter step2 - bridge limits and fees in OFTAdapter#10
Feature: oft adapter step2 - bridge limits and fees in OFTAdapter#10
Conversation
…gic and update dependencies
…ipient configuration
…contracts by removing bridge limits and minting logic, enhancing clarity and maintainability
…ts, and related logic to enhance clarity and reduce complexity
…cluding Hardhat version upgrades and new remapping file
…ncluding removal of outdated scripts and addition of remapping file
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
GoodDollarOFTAdapter._credittherequestIdis derived fromblock.timestamp,_to,_srcEid, and_amount, which makes it impossible to deterministically pre-approve off-chain (you can’t know the timestamp ahead of time); consider either passing a requestId from the message payload or changing the approval model so it’s usable in practice. - The
BridgeFeesstruct includesminFeeandmaxFee, but_takeFeeand_creditcurrently only usefeeand ignore the min/max fields; either enforce these bounds in the fee calculation or remove the unused fields to avoid confusion. - The
approvedRequestsmapping inGoodDollarOFTAdapteris write-only and never cleared, so approved entries will accumulate indefinitely; consider adding a mechanism to delete or expire approvals after use to avoid unbounded storage growth.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GoodDollarOFTAdapter._credit` the `requestId` is derived from `block.timestamp`, `_to`, `_srcEid`, and `_amount`, which makes it impossible to deterministically pre-approve off-chain (you can’t know the timestamp ahead of time); consider either passing a requestId from the message payload or changing the approval model so it’s usable in practice.
- The `BridgeFees` struct includes `minFee` and `maxFee`, but `_takeFee` and `_credit` currently only use `fee` and ignore the min/max fields; either enforce these bounds in the fee calculation or remove the unused fields to avoid confusion.
- The `approvedRequests` mapping in `GoodDollarOFTAdapter` is write-only and never cleared, so approved entries will accumulate indefinitely; consider adding a mechanism to delete or expire approvals after use to avoid unbounded storage growth.
## Individual Comments
### Comment 1
<location> `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:183-190` </location>
<code_context>
+ * @param amount The amount to calculate fee from
+ * @return fee The calculated fee amount
+ */
+ function _takeFee(uint256 amount) internal view returns (uint256 fee) {
+ fee = (amount * bridgeFees.fee) / 10000;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Bridge fee calculation ignores `minFee`/`maxFee` fields of `BridgeFees`.
`BridgeFees` exposes `minFee` and `maxFee`, but `_takeFee` only uses the BPS value and never enforces these bounds. This can mislead integrators into assuming on-chain caps that don’t exist.
If these fields should be enforced, consider clamping the computed fee, e.g.:
```solidity
uint256 raw = (amount * bridgeFees.fee) / 10000;
if (bridgeFees.minFee > 0 && raw < bridgeFees.minFee) fee = bridgeFees.minFee;
else if (bridgeFees.maxFee > 0 && raw > bridgeFees.maxFee) fee = bridgeFees.maxFee;
else fee = raw;
```
If they’re informational only, consider renaming or removing them to avoid implying enforcement in this function.
```suggestion
/**
* @notice Calculates the fee amount from the given amount
* @param amount The amount to calculate fee from
* @return fee The calculated fee amount
*/
function _takeFee(uint256 amount) internal view returns (uint256 fee) {
uint256 raw = (amount * bridgeFees.fee) / 10000;
uint256 minFee = bridgeFees.minFee;
uint256 maxFee = bridgeFees.maxFee;
if (minFee > 0 && raw < minFee) {
fee = minFee;
} else if (maxFee > 0 && raw > maxFee) {
fee = maxFee;
} else {
fee = raw;
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… by integrating IMessagePassingBridge structures, enhancing modularity and clarity
Feature: oft adapter step2 - bridge limits and fees in OFTAdapter
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
…d enhance GoodDollarMinterBurner and GoodDollarOFTAdapter contracts with storage gap for future upgrades
…er for enhanced bridge fee management
sirpy
left a comment
sirpy
left a comment
There was a problem hiding this comment.
There are no tests. If you would have added tests then you would see _enforcelimits isnt used
…rts in bridge contracts
…lidating parameters and ensuring checks on both sending and receiving sides
…GoodDollarOFTAdapter contract
…llarOFTAdapter contract for improved fee management
…ed bridge limits management for improved cross-chain transfer functionality
…FTAdapter for improved modularity and clarity in bridge limits management
… in GoodDollarOFTAdapter for improved clarity and maintainability of bridge limits and account tracking
|
@copilot please review the code
|
Agent-Logs-Url: https://github.com/GoodDollar/GoodBridge/sessions/f6728e75-162b-4893-bd60-29ea0f9abee9 Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
… check; add GitHub Action CI workflow Agent-Logs-Url: https://github.com/GoodDollar/GoodBridge/sessions/f6728e75-162b-4893-bd60-29ea0f9abee9 Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
Agent-Logs-Url: https://github.com/GoodDollar/GoodBridge/sessions/f6728e75-162b-4893-bd60-29ea0f9abee9 Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
…ransient branch triggers Agent-Logs-Url: https://github.com/GoodDollar/GoodBridge/sessions/f6728e75-162b-4893-bd60-29ea0f9abee9 Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
Added in commit 29d1f27:
Tests have been added in commit 99eeaa5. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
… all tests on any PR Agent-Logs-Url: https://github.com/GoodDollar/GoodBridge/sessions/adfd53e3-a3cd-4b0c-bd7b-e43a6fd84bf3 Co-authored-by: sirpy <671095+sirpy@users.noreply.github.com>
Fixed in commit 92451a1: Test failures: The The harness ( Workflow changes:
|
|
@sirpy |
…ollarOFT; add new test script in package.json
|
@sirpy, Changes are ready for review. |
…ure correct decimal conversion rate
…er to reset request status instead of deletion
|
@sirpy, Could you please review |
| .withArgs(adapter.address, true); | ||
| }); | ||
|
|
||
| it("operators can mint/burn; burn requires allowance", async () => { |
There was a problem hiding this comment.
for permissions you ALWAYS need to test also the negative
There was a problem hiding this comment.
I will add negative permission checks
| require(IERC20Metadata(_token).decimals() == IERC20Metadata(_token).decimals(), 'token decimals mismatch'); | ||
| uint8 sharedDecimals = OFTCoreUpgradeable.sharedDecimals(); | ||
| uint8 tokenDecimals = IERC20Metadata(_token).decimals(); | ||
| require(10 ** (tokenDecimals - sharedDecimals) == decimalConversionRate, 'token decimals mismatch'); |
There was a problem hiding this comment.
why is this required?
where is decimalConversionRate defined?
There was a problem hiding this comment.
In the constructor, the innerToken(param named as _token) address is used to calculate decimalConversionRate within OFTCoreUpgradeable based on that token’s decimals.
Later, the initialize method receives the innerToken address, so its decimals must match the token's decimals used during construction; otherwise, amount calculations in OFTCoreUpgradeable will break.
I made a small change to make it more direct:
Stored the constructor token’s decimals in innerTokenDecimals
Added a check in initialize to verify the provided token has matching decimals before proceeding
| run: yarn build | ||
| working-directory: packages/bridge-contracts | ||
|
|
||
| - name: Run all tests |
There was a problem hiding this comment.
all tests should run.
i've fixed the failing tests of other contracts
…n decimal validation; update tests for minting and burning authorization
|
tests for other contracts are still failing @sirpy |
Description
This PR inclues smart contracts which implementes bridge limits and fees in GoodDollarOFTAdapter.sol
About #7
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: