feat(solidity): bridge fee quote contract #734
Conversation
WalkthroughThis pull request introduces three new smart contracts: Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 16
🧹 Outside diff range and nitpick comments (7)
solidity/contracts/test/BridgeFeeQuoteTest.sol (2)
1-4: Consider specifying a more appropriate license and addressing the linter disable comment.
The
UNLICENSEDSPDX identifier is typically used for private or proprietary code. If this contract is intended to be open source, consider using a more appropriate open-source license.The
solhint-disable custom-errorscomment is disabling a linter rule. It's generally better to address linter warnings rather than disabling them. Consider implementing custom errors instead of disabling the linter rule.
1-31: Overall, the contract is well-structured but could benefit from some refinements.The
BridgeFeeQuoteTestcontract provides a simple implementation for managing oracle statuses. It's generally well-structured and serves its purpose as a test contract. However, consider the following improvements:
- Choose a more appropriate license if this is intended to be open-source.
- Address the linter disable comment by implementing custom errors.
- Add access control to the
setOraclefunction if this contract evolves beyond testing.- Remove or document the unused string parameters in
hasOracleandisOracleOnline.- Add comments explaining the purpose of each function and any non-obvious implementation details.
These changes will improve the contract's clarity, maintainability, and readiness for potential future use beyond testing.
solidity/test/bridge_fee_quote.ts (3)
28-29: Clarify the use of signers as tokensUsing signer addresses (
token1.address,token2.address) as token addresses may be confusing. Consider using mock token contracts or renaming variables for clarity.Example:
- [owner, token1, token2] = await ethers.getSigners(); + [owner, signer1, signer2] = await ethers.getSigners(); + const mockTokenFactory = await ethers.getContractFactory("MockToken"); + const token1 = await mockTokenFactory.deploy(); + const token2 = await mockTokenFactory.deploy();
172-176: Avoid reusing signer addresses as token addressesIn this loop, signer addresses are used as token addresses. It's better to deploy mock token contracts to represent different tokens.
Consider modifying the loop to deploy mock tokens:
let tokens: AddressLike[] = []; for (let i = 0; i < number; i++) { - tokens.push(singers[i + 10].address); + const mockToken = await mockTokenFactory.deploy(); + tokens.push(mockToken.address); }
166-166: Improve test description for clarityThe test description
"test 1 ~ 5 quote gas limit"is unclear. A more descriptive title enhances readability.Update the test description:
- it("test 1 ~ 5 quote gas limit", async function () { + it("should handle multiple quotes efficiently", async function () {solidity/contracts/bridge/BridgeFeeQuote.sol (1)
309-323: Remove unused functionhasActiveQuoteor implement its usageThe function
hasActiveQuoteis defined but not used within the contract. If it's not required, consider removing it to simplify the codebase. If it will be used in the future, add a comment explaining its intended purpose.solidity/contracts/bridge/BridgeFeeOracle.sol (1)
8-8: Ensure consistency between interface names and file namesThe imported interface
IBridgeFeeOracleis located in a file namedIBridgeFee.sol. For better maintainability and to avoid confusion, consider renaming the file toIBridgeFeeOracle.solto match the interface name.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- solidity/contracts/bridge/BridgeFeeOracle.sol (1 hunks)
- solidity/contracts/bridge/BridgeFeeQuote.sol (1 hunks)
- solidity/contracts/bridge/IBridgeFee.sol (1 hunks)
- solidity/contracts/test/BridgeFeeQuoteTest.sol (1 hunks)
- solidity/test/bridge_fee_quote.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
solidity/contracts/test/BridgeFeeQuoteTest.sol (4)
7-10: LGTM: OracleState struct is well-defined.The
OracleStatestruct is simple, clear, and appropriate for its purpose. The use of boolean fields forregisteredandonlinestatuses is a good choice for representing binary states.
12-12: LGTM: oracleStatus mapping is well-defined.The
oracleStatusmapping is appropriately defined as public, allowing efficient lookup of oracle states by address. The naming is clear and consistent with theOracleStatestruct it uses.
18-30:⚠️ Potential issueRemove unused parameter and consider adding comments for clarity.
The
hasOracleandisOracleOnlinefunctions are correctly implemented to return the respective status from theoracleStatusmapping. However, there's an unusedstringparameter in both functions. This could indicate a missing implementation detail or a leftover from a previous design.
- Consider removing the unused
stringparameter from both functions if it's not needed.- If the parameter is intended for future use or has a specific purpose, add a comment explaining its intended use to improve code clarity.
Example refactor for
hasOracle(apply similar changes toisOracleOnline):function hasOracle( - string memory, address _externalAddress ) external view returns (bool _result) { return oracleStatus[_externalAddress].registered; }#!/bin/bash # Check if there are any other unused parameters in the contract ast-grep --lang solidity --pattern 'function $FUNC_NAME($PARAMS) $MODIFIERS { $$$ }' solidity/contracts/test/BridgeFeeQuoteTest.sol | rg -i 'function.*memory.*{'
14-16: LGTM: setOracle function is correctly implemented, but consider access control.The
setOraclefunction correctly updates theoracleStatusmapping. However, note that there's no access control on this function. While this might be intentional for a test contract, in a production environment, you'd want to restrict who can call this function to prevent unauthorized changes to oracle statuses.solidity/test/bridge_fee_quote.ts (2)
245-248: Review the message hashing mechanismThe custom hashing with
solidityPackedKeccak256and manual prefixing may not comply with Ethereum's standard message signing. Usingwallet.signMessagehandles the hashing and prefixing appropriately.To confirm the correct message is being signed, consider outputting the message and comparing it with expected values.
251-254: Verify the correctness of 'v' value in the signatureThe handling of the
vvalue might be incorrect. Ensure that thevcomponent of the signature corresponds accurately to the expected values (27 or 28).Run the following script to confirm the
vvalues:This script checks the
vvalue in a standard signature for comparison.
Co-authored-by: todd <81545601+todd-woko@users.noreply.github.com>
Summary by CodeRabbit
New Features
BridgeFeeOracleandBridgeFeeQuotesmart contracts for managing oracle activity and fee quotes in a blockchain bridge context.Bug Fixes
Tests
BridgeFeeQuotecontract, ensuring robust functionality and reliability.