Feat/basic order contract #1072
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes significant changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (14)
solidity/orders/package.json (1)
16-16: Consider the implications of --via-ir flagAdding the
--via-irflag enables the IR-based pipeline in the Solidity compiler, which can provide better optimization but may increase compilation time. This is a good practice for production builds but consider making it configurable for development.Consider adding a separate script for development testing without --via-ir:
"scripts": { "test": "forge test --via-ir", + "test:dev": "forge test", "test:coverage": "forge coverage",solidity/orders/test/mocks/MockWardenPrecompile.sol (2)
1-5: Consider relaxing the Solidity version constraintThe version constraint
>=0.8.25 <0.9.0is very recent and might cause compatibility issues. Consider using a more established version like^0.8.0unless there are specific features from 0.8.25 that are required.
29-31: Enhance addKey implementationThe function should emit an event when updating the key status for better testability and observability.
function addKey(uint64 keyId, bool isGood) external { goodKeys[keyId] = isGood; + emit KeyStatusUpdated(keyId, isGood); }solidity/orders/test/mocks/MockSlinkyPrecompile.sol (2)
1-4: Consider relaxing the Solidity version constraintThe version constraint
>=0.8.25 <0.9.0is very narrow and requires a recent compiler version. Since this is a mock contract for testing, consider relaxing it to^0.8.0unless specific 0.8.25 features are needed.-pragma solidity >=0.8.25 <0.9.0; +pragma solidity ^0.8.0;
36-42: Enhance setPrice with validation and eventsAdd price validation and emit an event for better test observability.
+ event PriceSet(string base, string quote, uint256 price); + function setPrice( string calldata base, string calldata quote, uint256 price ) external { + if (bytes(base).length == 0 || bytes(quote).length == 0) { + revert InvalidInput(); + } prices[base][quote] = price; + emit PriceSet(base, quote, price); }solidity/orders/src/Types.sol (2)
24-27: Consider adding EQUAL condition and documentationThe
PriceConditionenum could benefit from additional conditions and documentation.Consider:
+/// @notice Defines price comparison conditions for order execution enum PriceCondition { + /// @notice Less than or equal to LTE, + /// @notice Greater than or equal to GTE, + /// @notice Exactly equal to + EQUAL }
44-48: Add field documentation and consider gas parametersThe struct lacks documentation explaining the purpose of each field and when they should be used.
Add NatSpec documentation:
+/// @notice Transaction fields defined by the order creator struct CreatorDefinedTxFields { + /// @notice Amount of native tokens to send with the transaction uint256 value; + /// @notice Target chain ID for the transaction uint256 chainId; + /// @notice Destination address for the transaction address to; }solidity/orders/test/BasicOrder.t.sol (2)
137-143: Follow consistent naming conventionsFunction names should follow a consistent convention. Current mix of camelCase and snake_case in test names makes the code less readable.
- function test_basicOrderRevertWhenConditionNotMet() public { + function test_BasicOrder_RevertWhenConditionNotMet() public {
145-155: Track TODOs and improve test coverageThe commented-out test cases indicate missing coverage for important scenarios:
- Oracle integration tests
- Unauthorized access tests
- Successful execution tests
Consider creating GitHub issues to track these pending test implementations.
Would you like me to help create GitHub issues to track these missing test cases?
solidity/orders/src/OrderFactory.sol (1)
45-45: Typo in comment: correct 'regisry' to 'registry'There is a typographical error in the comment. 'regisry' should be corrected to 'registry'.
Apply this diff to fix the typo:
- // TODO: register in regisry + // TODO: register in registrysolidity/orders/src/IExecution.sol (4)
47-50: Typographical error in Natspec commentThe Natspec comment for
setByAIServicehas a typo. The word "whenever" should be "whether" to accurately convey the intended meaning.Suggested fix:
* @param data Data from AI service. - * @return success A boolean value indicating whenever data was processed correctly. + * @return success A boolean value indicating whether data was processed correctly.
37-38: Clarify Natspec comment for 'calledByScheduler' functionThe Natspec comment for
calledByScheduleris ambiguous and could be clearer.Suggested revision:
/** - * @dev Indicates if this contract intended to be called by scheduler. + * @dev Indicates whether the contract was called by the scheduler.
42-43: Clarify Natspec comment for 'calledByAIService' functionThe Natspec comment for
calledByAIServiceis ambiguous and could be improved for clarity.Suggested revision:
/** - * @dev Indicates if this contract intended to be called by AI service. + * @dev Indicates whether the contract was called by the AI service.
54-54: Improve Natspec comment for 'isExecuted' functionThe Natspec comment for
isExecutedcan be rephrased for better clarity.Suggested revision:
/** - * @dev Indicates if execution was executed already. + * @dev Indicates whether the execution has already occurred.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
solidity/orders/package.json(1 hunks)solidity/orders/remappings.txt(1 hunks)solidity/orders/script/Deploy.s.sol(0 hunks)solidity/orders/src/BasicOrder.sol(1 hunks)solidity/orders/src/Foo.sol(0 hunks)solidity/orders/src/IExecution.sol(1 hunks)solidity/orders/src/OrderFactory.sol(1 hunks)solidity/orders/src/Types.sol(1 hunks)solidity/orders/test/BasicOrder.t.sol(1 hunks)solidity/orders/test/Foo.t.sol(0 hunks)solidity/orders/test/mocks/MockSlinkyPrecompile.sol(1 hunks)solidity/orders/test/mocks/MockWardenPrecompile.sol(1 hunks)
💤 Files with no reviewable changes (3)
- solidity/orders/script/Deploy.s.sol
- solidity/orders/src/Foo.sol
- solidity/orders/test/Foo.t.sol
✅ Files skipped from review due to trivial changes (1)
- solidity/orders/remappings.txt
🔇 Additional comments (6)
solidity/orders/package.json (2)
11-11: Verify the src directory specification in build command
The build command now explicitly specifies the src directory. While this is more precise, ensure all contract files are within this directory structure.
#!/bin/bash
# Description: Verify all Solidity files are in the correct directory
# List all Solidity files and their locations
echo "Checking Solidity file locations..."
fd -e sol --exclude "test/" --exclude "script/"
# Specifically check if any contracts exist outside src/
echo "Checking for contracts outside src/..."
fd -e sol --exclude "test/" --exclude "script/" --exclude "src/"1-10:
Verify the removal of @openzeppelin/contracts dependency
The removal of @openzeppelin/contracts is concerning since this PR introduces new contract code. OpenZeppelin provides critical security-focused implementations and is typically essential for smart contract development.
solidity/orders/test/mocks/MockSlinkyPrecompile.sol (1)
7-8: LGTM!
The nested mapping is well-structured for currency pair price storage with appropriate visibility.
solidity/orders/src/Types.sol (1)
1-2: Consider broadening the Solidity version constraint
The version constraint >=0.8.25 <0.9.0 is very narrow and requires a very recent Solidity version. This might cause compatibility issues with other contracts or development tools that haven't been updated to support 0.8.25 yet.
Consider using a more flexible range like ^0.8.0 unless there are specific features from 0.8.25 that are required.
solidity/orders/test/BasicOrder.t.sol (1)
2-2: Verify Solidity version compatibility
The specified Solidity version (>=0.8.25) is very recent and might not be widely supported yet. Consider if a lower minimum version would suffice for your needs while maintaining better compatibility.
✅ Verification successful
Solidity version constraint is consistent with project standards
The version constraint >=0.8.25 <0.9.0 is consistently used across all contracts in the solidity/orders/ directory, which is the main project scope. The precompiles interfaces use a lower version (>=0.8.18), but this is appropriate as they are external interfaces.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other contracts in the project use different Solidity versions
rg -g "*.sol" "pragma solidity"
Length of output: 831
solidity/orders/src/IExecution.sol (1)
30-34: Verify the necessity of including both 'gasPrice' and EIP-1559 parameters
The execute function includes parameters for gasPrice, maxPriorityFeePerGas, and maxFeePerGas. With EIP-1559 implemented, gasPrice is generally replaced by maxPriorityFeePerGas and maxFeePerGas. Please verify if all these parameters are necessary, or if gasPrice can be deprecated.
Run the following script to check the usage of gas parameters:
✅ Verification successful
Both gasPrice and EIP-1559 parameters are necessary for backward compatibility
The codebase shows that both legacy gasPrice and EIP-1559 parameters (maxPriorityFeePerGas, maxFeePerGas) are actively used across different components:
- The Solidity contracts use all parameters in their execution flow (see
BasicOrder.sol,Types.sol) - The frontend components (
spaceward/src/features/modals) handle both types of gas parameters - Backend services support both transaction types (see
warden/x/warden/types/v1beta2/sign_method_handler_ethereum.go)
This dual support is important for compatibility with both pre and post EIP-1559 networks, making it a necessary design choice rather than a redundancy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'gasPrice' is required alongside EIP-1559 parameters.
# Search for 'gasPrice' usage in the codebase
rg 'gasPrice' --type solidity
# Search for 'maxPriorityFeePerGas' and 'maxFeePerGas' usage
rg 'maxPriorityFeePerGas' --type solidity
rg 'maxFeePerGas' --type solidity
Length of output: 204
Script:
#!/bin/bash
# Description: Verify if 'gasPrice' is required alongside EIP-1559 parameters.
# Search for 'gasPrice' usage in the codebase
rg "gasPrice"
# Search for 'maxPriorityFeePerGas' and 'maxFeePerGas' usage
rg "maxPriorityFeePerGas"
rg "maxFeePerGas"
# Search for any implementations of the execute function
ast-grep --pattern 'function execute($$$) $$$'
Length of output: 4454
| mapping(uint64 keyId => KeyResponse keyResponse) private keys; | ||
| mapping(uint64 keyId => bool isGood) private goodKeys; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add events for state changes and implement keys mapping
- The
keysmapping is declared but never written to, makingkeyByIdalways return empty values. - Consider adding events for state changes in
addKeyfor better observability.
contract MockWardenPrecompile {
mapping(uint64 keyId => KeyResponse keyResponse) private keys;
mapping(uint64 keyId => bool isGood) private goodKeys;
+
+ event KeyStatusUpdated(uint64 indexed keyId, bool isGood);
+ event KeyAdded(uint64 indexed keyId, KeyResponse keyResponse);
+
+ function setKey(uint64 keyId, KeyResponse calldata response) external {
+ keys[keyId] = response;
+ emit KeyAdded(keyId, response);
+ }Committable suggestion skipped: line range outside the PR's diff.
| function keyById(uint64 id, int32[] calldata) external view returns (KeyResponse memory key) { | ||
| return keys[id]; | ||
| } | ||
|
|
||
| function newSignRequest( | ||
| uint64 keyId, | ||
| bytes calldata, | ||
| bytes[] calldata, | ||
| bytes calldata, | ||
| Types.Coin[] calldata, | ||
| uint64, | ||
| uint64, | ||
| string calldata, | ||
| string calldata | ||
| ) external view returns (bool isGood) { | ||
| isGood = goodKeys[keyId]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve mock implementation accuracy
keyByIdshould probably revert for non-existent keys to better simulate the real precompile behavior.newSignRequestignores most parameters, which might not accurately represent the real implementation's validation logic.
- function keyById(uint64 id, int32[] calldata) external view returns (KeyResponse memory key) {
- return keys[id];
+ function keyById(uint64 id, int32[] calldata) external view returns (KeyResponse memory key) {
+ key = keys[id];
+ require(key.pubKey.length > 0, "Key not found");
+ return key;
}📝 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.
| function keyById(uint64 id, int32[] calldata) external view returns (KeyResponse memory key) { | |
| return keys[id]; | |
| } | |
| function newSignRequest( | |
| uint64 keyId, | |
| bytes calldata, | |
| bytes[] calldata, | |
| bytes calldata, | |
| Types.Coin[] calldata, | |
| uint64, | |
| uint64, | |
| string calldata, | |
| string calldata | |
| ) external view returns (bool isGood) { | |
| isGood = goodKeys[keyId]; | |
| } | |
| function keyById(uint64 id, int32[] calldata) external view returns (KeyResponse memory key) { | |
| key = keys[id]; | |
| require(key.pubKey.length > 0, "Key not found"); | |
| return key; | |
| } | |
| function newSignRequest( | |
| uint64 keyId, | |
| bytes calldata, | |
| bytes[] calldata, | |
| bytes calldata, | |
| Types.Coin[] calldata, | |
| uint64, | |
| uint64, | |
| string calldata, | |
| string calldata | |
| ) external view returns (bool isGood) { | |
| isGood = goodKeys[keyId]; | |
| } |
| import {IWarden, KeyResponse} from "precompile-warden/IWarden.sol"; | ||
| import {Types} from "precompile-common/Types.sol"; | ||
|
|
||
| contract MockWardenPrecompile { |
There was a problem hiding this comment.
💡 Codebase verification
Mock implementation is incomplete
The MockWardenPrecompile contract only implements 3 methods (keyById, newSignRequest, and addKey) out of the 29 methods defined in the IWarden interface. This significant gap in implementation suggests that:
- The mock is likely implementing only the minimal functionality needed for specific test cases
- Many interface methods are not being tested through this mock
- The mock includes a non-interface method (
addKey) which appears to be test-specific
Key missing methods include:
- Critical keychain management:
addKeychainAdmin,removeKeychainAdmin,newKeychain - Space management:
newSpace,addSpaceOwner,updateSpace - Request handling:
fulfilKeyRequest,rejectKeyRequest,fulfilSignRequest - Query methods:
allKeys,spaces,keychains
🔗 Analysis chain
Verify interface implementation completeness
Let's verify if this mock implements all necessary methods from the IWarden interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and compare interface methods with mock implementation
ast-grep --pattern 'interface IWarden {
$$$
function $name($$$) $$$;
$$$
}'
Length of output: 83
Script:
#!/bin/bash
# Let's first check if we can find the IWarden interface file
fd IWarden.sol
# Then let's examine both the interface and mock implementation
echo "=== IWarden Interface ==="
rg "function" -A 1 $(fd IWarden.sol) || true
echo -e "\n=== MockWardenPrecompile Implementation ==="
rg "function" -A 1 solidity/orders/test/mocks/MockWardenPrecompile.sol || true
Length of output: 2325
| if(price == 0) { | ||
| string memory s1 = string.concat("no price / nonce reported for CurrencyPair: ", base); | ||
| string memory s2 = string.concat(s1, "/"); | ||
| string memory s3 = string.concat(s2, quote); | ||
| string memory s4 = string.concat(s3, ", the module is not tracking this CurrencyPair"); | ||
| revert(s4); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize error message construction
The current string concatenation approach is gas-inefficient. Consider using a more efficient error handling mechanism.
- string memory s1 = string.concat("no price / nonce reported for CurrencyPair: ", base);
- string memory s2 = string.concat(s1, "/");
- string memory s3 = string.concat(s2, quote);
- string memory s4 = string.concat(s3, ", the module is not tracking this CurrencyPair");
- revert(s4);
+ revert PriceNotFound(base, quote);Add this custom error at the contract level:
error PriceNotFound(string base, string quote);| function getPrice( | ||
| string calldata base, | ||
| string calldata quote | ||
| ) external view returns (GetPriceResponse memory response) { | ||
| uint256 price = prices[base][quote]; | ||
| if(price == 0) { | ||
| string memory s1 = string.concat("no price / nonce reported for CurrencyPair: ", base); | ||
| string memory s2 = string.concat(s1, "/"); | ||
| string memory s3 = string.concat(s2, quote); | ||
| string memory s4 = string.concat(s3, ", the module is not tracking this CurrencyPair"); | ||
| revert(s4); | ||
| } | ||
|
|
||
| QuotePrice memory quotePrice = QuotePrice({ | ||
| blockHeight: 0, | ||
| blockTimestamp: 0, | ||
| price: price | ||
| }); | ||
|
|
||
| response = GetPriceResponse ({ | ||
| id: 0, | ||
| nonce: 0, | ||
| decimals: 9, | ||
| price: quotePrice | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add input validation and consider parameterizing decimals
The function needs additional validation and improvements:
- Missing validation for empty base/quote strings
- Hardcoded decimals value should be configurable
+ uint8 public immutable decimals;
+
+ constructor(uint8 _decimals) {
+ decimals = _decimals;
+ }
+
function getPrice(
string calldata base,
string calldata quote
) external view returns (GetPriceResponse memory response) {
+ if (bytes(base).length == 0 || bytes(quote).length == 0) {
+ revert InvalidInput();
+ }
// ... rest of the function ...
response = GetPriceResponse ({
id: 0,
nonce: 0,
- decimals: 9,
+ decimals: decimals,
price: quotePrice
});
}Committable suggestion skipped: line range outside the PR's diff.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
solidity/orders/package.json (1)
19-19: Consider the tradeoffs of enabling IR-based codegenAdding the --via-ir flag enables the IR-based codegen pipeline, which can produce more optimized bytecode but may increase compilation time. This is a good optimization for production builds, but consider making it configurable for development workflows.
Consider splitting the test script into development and production variants:
- "test": "forge test --via-ir", + "test": "forge test", + "test:prod": "forge test --via-ir",solidity/orders/test/BasicOrder.t.sol (2)
39-71: RefactorbeforeTestSetupFunction to Reduce Code DuplicationThe
beforeTestSetupfunction contains repetitive code blocks for different test selectors. Consider refactoring to reduce duplication and enhance maintainability.Apply this diff to refactor the function:
function beforeTestSetup( bytes4 testSelector ) public pure returns (bytes[] memory beforeTestCalldata) { beforeTestCalldata = new bytes[](1); - if (testSelector == this.test_BasicOrder_StateBeforeExecution.selector) { - beforeTestCalldata[0] = abi.encodeWithSignature( - "test_BasicOrder_Create(bool,uint8)", - true, - Types.PriceCondition.LTE); - } else if (testSelector == this.test_basicOrderRevertWhenConditionNotMet.selector) { - beforeTestCalldata[0] = abi.encodeWithSignature( - "test_BasicOrder_Create(bool,uint8)", - true, - Types.PriceCondition.LTE); - } else if (testSelector == this.test_basicOrderRevertWhenUnauthorized.selector) { - beforeTestCalldata[0] = abi.encodeWithSignature( - "test_BasicOrder_Create(bool,uint8)", - true, - Types.PriceCondition.LTE); - } else if (testSelector == this.test_basicOrderExecuteWhenPriceMovesDown.selector) { - beforeTestCalldata[0] = abi.encodeWithSignature( - "test_BasicOrder_Create(bool,uint8)", - true, - Types.PriceCondition.LTE); - } else if (testSelector == this.test_basicOrderExecuteWhenPriceMovesUp.selector) { - beforeTestCalldata[0] = abi.encodeWithSignature( - "test_BasicOrder_Create(bool,uint8)", - true, - Types.PriceCondition.GTE); - } + uint8 priceCondition = Types.PriceCondition.LTE; + if (testSelector == this.test_basicOrderExecuteWhenPriceMovesUp.selector) { + priceCondition = Types.PriceCondition.GTE; + } + beforeTestCalldata[0] = abi.encodeWithSignature( + "test_BasicOrder_Create(bool,uint8)", + true, + priceCondition + ); }
204-204: Use Meaningful Variables Instead of Magic Numbers inorder.executeIn the calls to
order.execute(1, 1, 1, 1, 1);, the arguments are all literal1s. Consider defining variables or constants with meaningful names to improve code clarity and maintainability.Also applies to: 223-223
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
solidity/orders/package.json(1 hunks)solidity/orders/remappings.txt(1 hunks)solidity/orders/src/BasicOrder.sol(1 hunks)solidity/orders/src/IExecution.sol(1 hunks)solidity/orders/src/OrderFactory.sol(1 hunks)solidity/orders/test/BasicOrder.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- solidity/orders/src/BasicOrder.sol
- solidity/orders/src/IExecution.sol
- solidity/orders/src/OrderFactory.sol
🔇 Additional comments (5)
solidity/orders/remappings.txt (2)
1-2: LGTM: Standard dependency mappings are correctly configured
The mappings for @openzeppelin/contracts and forge-std are properly configured to point to their respective locations in node_modules.
3-5: Verify precompile paths and consider using environment variables
The precompile mappings use relative paths that point outside the project directory. Let's verify these paths exist and consider making them more maintainable.
Consider:
- Using environment variables for precompile paths to make them configurable
- Adding documentation about the required precompile structure
- Including validation in the build process to ensure precompiles exist
✅ Verification successful
Let me try a different approach to verify the paths by searching within the repository structure.
Relative paths in remappings.txt are correct and valid
The repository structure shows that the precompile directories (precompiles/common/, precompiles/warden/, precompiles/slinky/) exist at the correct locations relative to solidity/orders/remappings.txt. The paths in remappings.txt (../../precompiles/...) are valid and follow the standard Solidity remapping convention. Using environment variables is not necessary here as:
- These are fixed paths relative to the project structure
- Remappings are a standard Solidity/Foundry feature for import resolution
- The paths are already maintainable within the monorepo context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of precompile directories and their content
# Check if the precompile directories exist and contain Solidity files
for dir in common warden slinky; do
echo "Checking precompiles/$dir:"
if [ -d "../../precompiles/$dir" ]; then
# List .sol files in the directory
find "../../precompiles/$dir" -name "*.sol" -type f
else
echo "Directory not found: ../../precompiles/$dir"
fi
done
Length of output: 719
Script:
#!/bin/bash
# Check the actual repository structure for precompiles
echo "Repository structure around precompiles:"
fd precompile
fd -t d slinky
fd -t d warden
fd -t d common
echo -e "\nChecking remappings.txt location and content:"
fd remappings.txt -x cat {}
Length of output: 1405
solidity/orders/package.json (2)
14-14: Verify the impact of src-specific build command
The build command now explicitly targets the src directory. While this is more specific, we should verify that no critical contract files exist outside the src directory.
#!/bin/bash
# Find all Solidity files outside src directory
echo "Solidity files outside src directory:"
fd -e sol -E "src/**"
# Check if there are any forge.toml or foundry.toml configurations that might be affected
fd "forge.toml|foundry.toml"9-11: Verify OpenZeppelin Contracts version compatibility and security
The addition of @openzeppelin/contracts v5.0.1 is significant for smart contract security. Let's verify its stability and security status.
✅ Verification successful
OpenZeppelin Contracts v5.0.1 usage is appropriate and minimal
The dependency is being used appropriately in BasicOrder.sol for the ReentrancyGuard functionality, which is a critical security feature. Version 5.0.1 is a stable release, and the implementation shows minimal and focused usage of OpenZeppelin's battle-tested security contracts.
solidity/orders/src/BasicOrder.solimports only the necessaryReentrancyGuardcontract- The dependency has been consistently managed in the package.json with no concerning version changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories and latest versions
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@openzeppelin/contracts") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check if any contracts in the project use OpenZeppelin imports
rg -l "import.*@openzeppelin/contracts" --type sol
Length of output: 835
Script:
#!/bin/bash
# Check for OpenZeppelin imports and their usage
rg -l "@openzeppelin/contracts" solidity/orders/src/
rg "@openzeppelin/contracts" solidity/orders/src/ -A 2
# Check if there are any previous versions in package.json history
git log -p solidity/orders/package.json | grep -A 5 -B 5 "@openzeppelin/contracts"
Length of output: 1318
solidity/orders/test/BasicOrder.t.sol (1)
191-191: Verify Usage of vm.broadcast(address(0))
Broadcasting from address(0) is unusual and may not function as intended. Please verify whether broadcasting from the zero address is appropriate in this context.
Run the following script to check for unintended usage of address(0):
✅ Verification successful
vm.broadcast(address(0)) is correctly used in unauthorized access test
The usage of address(0) with vm.broadcast is intentional and appropriate in this context. It's part of a test function test_basicOrderRevertWhenUnauthorized() that verifies the contract correctly reverts when called by an unauthorized address. Using address(0) is a common practice to test unauthorized access, and it's properly combined with vm.expectRevert to verify the reversion behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for instances of vm.broadcast(address(0)) in the codebase
# Test: Ensure that vm.broadcast is not used incorrectly with address(0).
rg 'vm\.broadcast\(address\(0\)\)'
Length of output: 107
Script:
#!/bin/bash
# Let's check the context around this broadcast to understand its usage
rg -B 5 -A 5 'vm\.broadcast\(address\(0\)\)' solidity/orders/test/BasicOrder.t.sol
Length of output: 432
| address(bytes20(bytes("0xeE567Fe1712Faf6149d80dA1E6934E354124CfE3"))); | ||
| address private constant RECEIVER = address(bytes20(bytes("0x18517Cb2779186B86b1F8947dFdB6078C1B9C9db"))); | ||
|
|
||
| event OrderCreated(address indexed orderCreator, OrderType indexed orderType, address orderContact); |
There was a problem hiding this comment.
Typographical Error in Event Declaration
The parameter name orderContact in the OrderCreated event should likely be orderContract.
Apply this diff to fix the typo:
-event OrderCreated(address indexed orderCreator, OrderType indexed orderType, address orderContact);
+event OrderCreated(address indexed orderCreator, OrderType indexed orderType, address orderContract);📝 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.
| event OrderCreated(address indexed orderCreator, OrderType indexed orderType, address orderContact); | |
| event OrderCreated(address indexed orderCreator, OrderType indexed orderType, address orderContract); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
solidity/orders/test/BasicOrder.t.sol (3)
63-63: Consider using a mock registry for clarityCurrently, the registry is set to
address(this)as a placeholder. Using a dedicated mock registry contract can improve the clarity of the test setup and make future maintenance easier.
134-134: MakechainIdconfigurable for flexibilityUsing a hardcoded
chainId(11_155_111) may cause issues when deploying or testing on different networks. Consider retrieving the chain ID dynamically usingblock.chainidor making it an input parameter to enhance flexibility.Apply this diff to use the dynamic chain ID:
- chainId: 11_155_111, + chainId: block.chainid,
151-156: Add assertions to enhance test robustnessCurrently, the
test_BasicOrder_StateBeforeExecutionfunction checks several state variables. To improve test robustness, consider adding assertions to verify that the order cannot be executed and has not been executed, reinforcing the expected initial state.Apply this diff to add more assertions:
function test_BasicOrder_StateBeforeExecution() public { + assertFalse(order.canExecute(), "Order should not be executable before conditions are met"); + assertFalse(order.isExecuted(), "Order should not be marked as executed"); assert(order.calledByScheduler()); assert(!order.calledByAIService()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
solidity/orders/package.json(1 hunks)solidity/orders/remappings.txt(1 hunks)solidity/orders/src/BasicOrder.sol(1 hunks)solidity/orders/src/IExecution.sol(1 hunks)solidity/orders/src/OrderFactory.sol(1 hunks)solidity/orders/src/Types.sol(1 hunks)solidity/orders/test/BasicOrder.t.sol(1 hunks)solidity/orders/test/mocks/MockSlinkyPrecompile.sol(1 hunks)solidity/orders/test/mocks/MockWardenPrecompile.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- solidity/orders/package.json
- solidity/orders/src/BasicOrder.sol
- solidity/orders/src/IExecution.sol
- solidity/orders/src/OrderFactory.sol
- solidity/orders/src/Types.sol
- solidity/orders/test/mocks/MockSlinkyPrecompile.sol
- solidity/orders/test/mocks/MockWardenPrecompile.sol
🔇 Additional comments (2)
solidity/orders/remappings.txt (2)
3-5: Verify precompile paths exist
The precompile mappings reference paths in the parent directory. Let's verify these paths exist and contain the expected contracts.
✅ Verification successful
Precompile paths verified successfully
The remappings are correctly configured. All referenced precompile paths exist and contain the expected Solidity interface files:
- precompile-common → precompiles/common/Types.sol
- precompile-warden → precompiles/warden/IWarden.sol
- precompile-slinky → precompiles/slinky/ISlinky.sol
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that precompile paths exist and contain Solidity contracts
# Check if the precompile directories exist and contain .sol files
for dir in "../../precompiles/common" "../../precompiles/warden" "../../precompiles/slinky"; do
echo "Checking $dir:"
if [ -d "$dir" ]; then
echo "Directory exists"
find "$dir" -name "*.sol" -type f
else
echo "Directory not found"
fi
done
Length of output: 260
Script:
#!/bin/bash
# Let's try a different approach using fd to find the precompile directories
echo "Searching for precompile directories:"
fd -t d "precompiles"
echo -e "\nSearching for Solidity files in precompiles:"
fd -e sol . "precompiles"
Length of output: 366
1-2: External dependency mappings look good
The mappings for @openzeppelin/contracts and forge-std follow the standard convention.
✅ Verification successful
External dependency mappings are correctly configured
The remappings in solidity/orders/remappings.txt are properly set up and actively used in the codebase:
@openzeppelin/contracts/is used for importingReentrancyGuardinBasicOrder.solforge-std/is used for importingTestin test files
The imports are consistent with the remapping paths and follow standard conventions for Foundry projects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify @openzeppelin version compatibility
# Check package.json for @openzeppelin version
if [ -f "package.json" ]; then
echo "OpenZeppelin version from package.json:"
jq '.dependencies["@openzeppelin/contracts"]' package.json
fi
# Look for OpenZeppelin imports in Solidity files
echo "Files importing OpenZeppelin:"
rg -l '@openzeppelin/contracts' --type sol
Length of output: 260
Script:
#!/bin/bash
# Let's verify the dependencies and imports more thoroughly
# Check if package.json exists and its content
echo "Checking package.json:"
if [ -f "package.json" ]; then
cat package.json
fi
# Check if node_modules/@openzeppelin exists
echo -e "\nChecking OpenZeppelin directory:"
ls -la node_modules/@openzeppelin 2>/dev/null || echo "OpenZeppelin directory not found"
# Check if node_modules/forge-std exists
echo -e "\nChecking forge-std directory:"
ls -la node_modules/forge-std 2>/dev/null || echo "forge-std directory not found"
# Search for Solidity files that might import these dependencies
echo -e "\nSearching for Solidity files:"
fd -e sol
# Check content of found Solidity files for imports
echo -e "\nChecking imports in Solidity files:"
fd -e sol -x cat {} \; 2>/dev/null
Length of output: 55095
| if (condition > 1) { | ||
| condition = 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance condition validation with stricter checks
The current condition check silently defaults to 0 when condition > 1. Consider using proper validation to ensure that only valid PriceCondition enums are accepted, and revert when an invalid condition is provided.
Apply this diff to improve the validation:
- if (condition > 1) {
- condition = 0;
- }
+ require(condition <= uint8(Types.PriceCondition.GTE), "Invalid condition");📝 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.
| if (condition > 1) { | |
| condition = 0; | |
| } | |
| require(condition <= uint8(Types.PriceCondition.GTE), "Invalid condition"); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
solidity/orders/README.md (3)
4-6: Add comma for better readabilityThe sentence structure would be clearer with a comma after "commands".
-Before using commands install dependencies with `yarn install`. +Before using commands, install dependencies with `yarn install`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...p contracts ### Commands Before using commands install dependencies with `yarn install...(AI_HYDRA_LEO_MISSING_COMMA)
8-12: Add more context about the build processThe build command documentation could be more helpful by explaining what gets built and where the output is stored.
#### Build ```sh yarn build
+This command builds the Solidity contracts and generates artifacts in the
outdirectory.--- `14-16`: **Enhance test documentation** The test section would benefit from additional information about the test suite and available options. ```diff #### Tests ```sh yarn test
+This command runs the full test suite for all contracts. For running specific tests, you can use:
+
+sh +yarn test -m <test_function_name> # Run a specific test +yarn test --match-contract <contract_name> # Run tests for a specific contract +</blockquote></details> <details> <summary>solidity/orders/src/OrderFactory.sol (1)</summary><blockquote> `57-57`: **Fix typo in comment** There's a typo in the comment: "regisry" should be "registry". Apply this diff to fix the typo: ```diff - // TODO: register in regisry + // TODO: register in registrysolidity/orders/test/BasicOrder.t.sol (2)
37-59: RefactorbeforeTestSetupto reduce code duplicationThe
beforeTestSetupfunction contains repetitive code blocks when initializingbeforeTestCalldata. Refactoring this function will improve maintainability and readability.Apply this diff to refactor the function:
function beforeTestSetup(bytes4 testSelector) public pure returns (bytes[] memory beforeTestCalldata) { + bytes memory encodedCalldata; if (testSelector == this.test_basicOrderExecuteWhenPriceMovesUp.selector) { - beforeTestCalldata = new bytes[](1); - beforeTestCalldata[0] = - abi.encodeWithSignature("test_BasicOrder_Create(bool,uint8)", true, Types.PriceCondition.GTE); + encodedCalldata = abi.encodeWithSignature("test_BasicOrder_Create(bool,uint8)", true, Types.PriceCondition.GTE); } else { - beforeTestCalldata = new bytes[](1); - beforeTestCalldata[0] = - abi.encodeWithSignature("test_BasicOrder_Create(bool,uint8)", true, Types.PriceCondition.LTE); + encodedCalldata = abi.encodeWithSignature("test_BasicOrder_Create(bool,uint8)", true, Types.PriceCondition.LTE); } + beforeTestCalldata = new bytes[](1); + beforeTestCalldata[0] = encodedCalldata; }
177-179: Specify event parameters invm.expectEmitfor precise event verificationCurrently,
vm.expectEmit(false, false, false, false);is used before emitting theExecutedevent. This configuration does not check any topics or data. To ensure the test accurately verifies that theExecutedevent is emitted with the correct parameters, consider setting the appropriate flags totrue.For example, if you want to check that the event is emitted without caring about topics or data, you can omit
vm.expectEmit. Otherwise, set at least one of the flags totrue:- vm.expectEmit(false, false, false, false); + vm.expectEmit(true, false, false, false); emit Executed();Also applies to: 197-199
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
solidity/orders/.prettierignore(2 hunks)solidity/orders/README.md(1 hunks)solidity/orders/package.json(1 hunks)solidity/orders/src/OrderFactory.sol(1 hunks)solidity/orders/test/BasicOrder.t.sol(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- solidity/orders/.prettierignore
🚧 Files skipped from review as they are similar to previous changes (1)
- solidity/orders/package.json
🧰 Additional context used
📓 Path-based instructions (1)
solidity/orders/README.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
solidity/orders/README.md
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...p contracts ### Commands Before using commands install dependencies with `yarn install...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
solidity/orders/src/OrderFactory.sol (1)
23-23: Typo in event parameter name orderContact
The parameter name orderContact in the OrderCreated event appears to be a typo. It should likely be orderContract to accurately represent the address of the created order contract.
solidity/orders/test/BasicOrder.t.sol (2)
61-89: Initialization in setUp function is appropriate
The setUp function correctly initializes the test environment, including deploying mock precompiles and setting up test data. This setup ensures that the tests run in a predictable environment.
85-85: Confirm the validity of the price pair
The price pair is set to { base: "ETH", quote: "DOGE" }. Ensure that DOGE is a valid and supported quote in your testing environment.
Run the following script to check the supported price pairs:
✅ Verification successful
DOGE is a valid quote currency in the test environment
The mock Slinky precompile implementation (MockSlinkyPrecompile.sol) uses a flexible mapping that can handle any base/quote pair, and the test suite successfully uses "DOGE" as a quote currency in multiple test cases:
- The price pair is properly initialized in the test setup
- Multiple test cases (
test_basicOrderExecuteWhenPriceMovesDown,test_basicOrderExecuteWhenPriceMovesUp) successfully set and use ETH/DOGE prices - The mock precompile correctly handles price updates for this pair
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if 'DOGE' is a supported quote currency in the mock precompile.
# Expected Result: 'DOGE' should be listed among the supported quotes.
# List supported quotes
ast-grep --pattern 'struct PricePair { base: $_; quote: $_; }' **/*.sol
Length of output: 131
Script:
#!/bin/bash
# Let's try a different approach to find price pair related structures and their usage
# First, let's find files that might contain price pair definitions
fd -e sol | xargs rg -l "PricePair|DOGE"
# Then let's look for any struct definitions or enums that might define supported currencies
fd -e sol | xargs rg "struct.*Price|enum.*Currency|DOGE" -A 5 -B 5
Length of output: 2534
Script:
#!/bin/bash
# Let's check the mock precompile implementation to see if it handles DOGE quotes
fd -e sol | xargs rg "mock.*Slinky|MockSlinky" -A 10 -B 10
# Also check for any test setup related to price pairs
fd -e sol | xargs rg "setUp.*mock.*Slinky|mock.*Price" -A 10 -B 10
Length of output: 15611
| ### Factory |
There was a problem hiding this comment.
Add content to empty sections
The "Order" and "Factory" sections are empty. These sections should include explanations of their respective components and usage examples.
Would you like me to help generate comprehensive documentation for these sections based on the contract implementations?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
solidity/orders/src/IExecution.sol (3)
4-10: Add NatSpec documentation for the ExecutionData struct and its fields.While the struct fields are well-organized and use appropriate types, adding documentation would improve code maintainability and integration experience.
+/// @notice Represents the data required for executing a transaction struct ExecutionData { + /// @notice The address initiating the execution address caller; + /// @notice The target address for the execution address to; + /// @notice The chain ID where the execution should occur uint256 chainId; + /// @notice The calldata for the execution bytes data; + /// @notice The amount of native tokens to send with the execution uint256 value; }
12-15: Add NatSpec documentation for the Caller enum.The enum values would benefit from documentation explaining their roles and usage context.
+/// @notice Defines the types of entities that can initiate executions enum Caller { + /// @notice Represents the automated scheduler system Scheduler, + /// @notice Represents the AI-based execution service AI }
34-42: Enhance documentation for execute function parameters.The gas-related parameters would benefit from more detailed documentation explaining their constraints and relationships.
/** * @dev Creates action for new sign request from stored order data. * If action created successfully then emit Executed event. * @param nonce The key account nonce. - * @param gas gas parameter in eth transaction. - * @param gasPrice gasPrice parameter in eth transaction. - * @param maxPriorityFeePerGas maxPriorityFeePerGas parameter in eth transaction. - * @param maxFeePerGas maxFeePerGas parameter in eth transaction. + * @param gas The amount of gas to allocate for the transaction execution + * @param gasPrice The legacy gas price in wei (only used for legacy transactions) + * @param maxPriorityFeePerGas The maximum priority fee per gas in wei (EIP-1559) + * @param maxFeePerGas The maximum total fee per gas in wei (EIP-1559) * @return A boolean value indicating parameter action was created. */solidity/orders/src/OrderFactory.sol (2)
14-17: Consider moving common errors to a separate file.Following the existing review suggestion, consider creating a separate
errors.solfile for common errors likeInvalidRegistryAddressandInvalidSchedulerAddress, while keeping contract-specific errors likeUnimplementedandUnsupportedOrderhere.
78-78: Implement registry registration.The TODO comment indicates missing functionality for registering the order in the registry.
Would you like me to help implement the registry registration functionality or create a GitHub issue to track this task?
solidity/orders/src/BasicOrder.sol (2)
25-25: Reorganize functions according to Solidity style guideThe contract should follow the recommended function ordering:
- constructor
- receive/fallback
- external
- public
- internal
- private
Consider reorganizing the functions in this order for better readability and maintainability.
165-174: Consider caching packed swap dataThe
executionDatafunction repacks swap data on every call using_packSwapData(). This is inefficient for frequently accessed data.Consider caching the packed data:
+ bytes private _cachedSwapData; + function executionData() external view returns (ExecutionData memory data) { - bytes memory d = _packSwapData(); data = ExecutionData({ caller: _keyAddress, to: orderData.creatorDefinedTxFields.to, chainId: orderData.creatorDefinedTxFields.chainId, value: orderData.creatorDefinedTxFields.value, - data: d + data: _cachedSwapData }); }solidity/orders/test/BasicOrder.t.sol (2)
59-92: Consider makingbeforeTestSetupfunctionprivateThe
beforeTestSetupfunction is currently declared aspublic, but it appears to be used only internally within the test contract. Changing its visibility toprivatecan improve encapsulation and prevent unintended external access.Apply this diff to adjust the function visibility:
-function beforeTestSetup(bytes4 testSelector) public pure returns (bytes[] memory beforeTestCalldata) { +function beforeTestSetup(bytes4 testSelector) private pure returns (bytes[] memory beforeTestCalldata) {
245-271: Consider makingsaveOrderDatafunctionprivateThe
saveOrderDatafunction is declared aspublic, but it seems to be used internally for setting up test data. Changing its visibility toprivateenhances encapsulation and aligns with best practices.Apply this diff to adjust the function visibility:
-function saveOrderData() public { +function saveOrderData() private {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
solidity/orders/src/BasicOrder.sol(1 hunks)solidity/orders/src/IExecution.sol(1 hunks)solidity/orders/src/OrderFactory.sol(1 hunks)solidity/orders/test/BasicOrder.t.sol(1 hunks)
🔇 Additional comments (10)
solidity/orders/src/IExecution.sol (2)
1-2: LGTM! Appropriate license and Solidity version constraints.
The file uses a specific version range that leverages recent Solidity security features while preventing compilation with potentially incompatible future versions.
54-54: Consider adding validation requirements for setByAIService.
The function should document any validation requirements or constraints for the input data to ensure proper integration.
solidity/orders/src/OrderFactory.sol (5)
1-7: LGTM! Well-structured imports and appropriate version constraints.
The pragma version is appropriately constrained, and imports are organized logically.
19-19: Fix typo in event parameter name.
The parameter name orderContact should be orderContract.
27-31: LGTM! Well-documented state variables.
The state variables are appropriately marked as public and well-documented with comments.
33-44: LGTM! Robust constructor with proper validations.
The constructor includes appropriate zero-address checks and proper initialization order.
46-61: Add input validation for order creation parameters.
As suggested in the past review, add validation to prevent creation of invalid orders.
solidity/orders/test/BasicOrder.t.sol (3)
55-57: Duplicate Comment: Consider making addresses configurable
The past review comment regarding hardcoded addresses for SEPOLIA_UNISWAP_V2_ROUTER and RECEIVER is still valid. Making these addresses configurable enhances test flexibility and prevents potential issues when testing on different networks.
129-131: Duplicate Comment: Improve condition validation
The previous suggestion to enhance condition validation is still applicable. Using a require statement for validation provides clearer error handling compared to silently defaulting the condition.
203-203: Duplicate Comment: Avoid using address(0) in vm.broadcast
The past comment about not using address(0) in vm.broadcast remains relevant. Using a specific unauthorized test address would simulate unauthorized access more effectively.
| /** | ||
| * @dev Returns the list of intended callers. | ||
| */ | ||
| function callers() external returns (Caller[] memory callersList); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add view modifiers to state-reading functions.
These functions don't modify state and should be marked as view:
- function callers() external returns (Caller[] memory callersList);
+ function callers() external view returns (Caller[] memory callersList);
- function isExecuted() external returns (bool);
+ function isExecuted() external view returns (bool);
- function executionData() external returns (ExecutionData memory executionData);
+ function executionData() external view returns (ExecutionData memory executionData);Also applies to: 59-59, 65-65
solidity/orders/src/OrderFactory.sol
Outdated
| function changeScheduler(address _scheduler) public onlyOwner { | ||
| emit SchedulerChanged(scheduler, _scheduler); | ||
| scheduler = _scheduler; | ||
| } |
There was a problem hiding this comment.
Add zero-address validation in changeScheduler.
The function should validate that the new scheduler address is not zero to maintain consistency with constructor checks.
function changeScheduler(address _scheduler) public onlyOwner {
+ if (_scheduler == address(0)) {
+ revert InvalidSchedulerAddress();
+ }
emit SchedulerChanged(scheduler, _scheduler);
scheduler = _scheduler;
}| function setByAIService(bytes calldata) external pure returns (bool success) { | ||
| success = false; | ||
| } |
There was a problem hiding this comment.
Implement or remove placeholder AI service function
The setByAIService function appears to be a placeholder that always returns false. This could lead to confusion about the contract's capabilities.
Either implement the function properly or remove it if not needed. If it's intentionally disabled, add a comment explaining why:
- function setByAIService(bytes calldata) external pure returns (bool success) {
- success = false;
- }
+ /// @dev AI service integration is not supported in basic orders
+ function setByAIService(bytes calldata) external pure returns (bool) {
+ revert("AI service integration not supported");
+ }📝 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.
| function setByAIService(bytes calldata) external pure returns (bool success) { | |
| success = false; | |
| } | |
| /// @dev AI service integration is not supported in basic orders | |
| function setByAIService(bytes calldata) external pure returns (bool) { | |
| revert("AI service integration not supported"); | |
| } |
solidity/orders/src/BasicOrder.sol
Outdated
| _wardenPrecompile = IWarden(IWARDEN_PRECOMPILE_ADDRESS); | ||
| KeyResponse memory keyResponse = _wardenPrecompile.keyById(_orderData.signRequestData.keyId, new int32[](0)); | ||
| _keyAddress = address(bytes20(keccak256(keyResponse.key.publicKey))); | ||
|
|
||
| _slinkyPrecompile = ISlinky(ISLINKY_PRECOMPILE_ADDRESS); | ||
| _slinkyPrecompile.getPrice(_orderData.pricePair.base, _orderData.pricePair.quote); |
There was a problem hiding this comment.
Add error handling for precompile contract calls
The constructor makes external calls to _wardenPrecompile and _slinkyPrecompile without handling potential failures. These calls could revert and make contract deployment impossible.
Add try-catch blocks or appropriate error handling:
- KeyResponse memory keyResponse = _wardenPrecompile.keyById(_orderData.signRequestData.keyId, new int32[](0));
+ try _wardenPrecompile.keyById(_orderData.signRequestData.keyId, new int32[](0)) returns (KeyResponse memory keyResponse) {
+ _keyAddress = address(bytes20(keccak256(keyResponse.key.publicKey)));
+ } catch {
+ revert("Failed to retrieve key from warden precompile");
+ }Committable suggestion skipped: line range outside the PR's diff.
| function execute( | ||
| uint256 nonce, | ||
| uint256 gas, | ||
| uint256 gasPrice, | ||
| uint256 maxPriorityFeePerGas, | ||
| uint256 maxFeePerGas | ||
| ) |
There was a problem hiding this comment.
Add validation for gas-related parameters
The execute function accepts gas-related parameters without validation. This could lead to failed transactions if these parameters are set incorrectly.
Add validation checks:
function execute(
uint256 nonce,
uint256 gas,
uint256 gasPrice,
uint256 maxPriorityFeePerGas,
uint256 maxFeePerGas
) external nonReentrant returns (bool) {
+ if (gas == 0) revert("Invalid gas value");
+ if (maxFeePerGas < maxPriorityFeePerGas) revert("Invalid fee values");
+ if (maxFeePerGas > 0 && gasPrice > 0) revert("Cannot specify both gasPrice and maxFeePerGas");Committable suggestion skipped: line range outside the PR's diff.
- save key address in order constructor - extend IExecution with executionData method
- fix typo - return order address from createOrder - move OrderType to out scope
- return ExecutionData struct from executionData
bc2d60d to
f3f3176
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
solidity/orders/README.md (1)
5-5: Add a comma for better readabilityConsider adding a comma after "commands" to improve readability.
-Before using commands install dependencies with `yarn install`. +Before using commands, install dependencies with `yarn install`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...p contracts ### Commands Before using commands install dependencies with `yarn install...(AI_HYDRA_LEO_MISSING_COMMA)
solidity/orders/src/OrderFactory.sol (3)
19-19: Add index toorderContactparameter in OrderCreated eventAdding an index to the
orderContactparameter will improve filtering capabilities when querying events.- event OrderCreated(address indexed orderCreator, OrderType indexed orderType, address indexed orderContact); + event OrderCreated(address indexed orderCreator, OrderType indexed orderType, address indexed orderContact);
14-17: Consider moving common errors to a separate fileFor better organization and reusability, consider moving generic errors like
InvalidRegistryAddressandInvalidSchedulerAddressto a separateerrors.solfile, while keeping contract-specific errors likeUnsupportedOrderhere.
78-78: Implement registry registrationThe TODO comment indicates missing registry registration functionality. This should be implemented to ensure proper order tracking.
Would you like me to help implement the registry registration logic?
solidity/orders/src/BasicOrder.sol (2)
11-22: Add NatSpec documentation for custom errorsAdding NatSpec documentation for custom errors would improve code maintainability and developer experience by clearly explaining when each error is thrown.
Example:
+/// @notice Thrown when the price condition for order execution is not met error ConditionNotMet(); +/// @notice Thrown when attempting to execute an already executed order error ExecutedError();
165-174: Cache packed swap data in executionDataThe function calls _packSwapData which performs encoding operations. Consider caching the result if this function is called frequently.
function executionData() external view returns (ExecutionData memory data) { - bytes memory d = _packSwapData(); + bytes memory d = _cachedSwapData != bytes(0) ? _cachedSwapData : _packSwapData(); data = ExecutionData({solidity/orders/test/BasicOrder.t.sol (4)
198-198: Replace magic numbers in_order.executecall with named constantsUsing magic numbers in the call
_order.execute(1, 1, 1, 1, 1);reduces readability. Define named constants or variables for these parameters to improve code clarity and maintainability.
216-216: Replace magic numbers in_order.executecall with named constantsUsing magic numbers in the call
_order.execute(1, 1, 1, 1, 1);reduces readability. Define named constants or variables for these parameters to improve code clarity and maintainability.
235-235: Replace magic numbers in_order.executecall with named constantsUsing magic numbers in the call
_order.execute(1, 1, 1, 1, 1);reduces readability. Define named constants or variables for these parameters to improve code clarity and maintainability.
128-128: Ensure consistent naming conventions in test function namesThe test function names are inconsistent in their casing after underscores, such as
test_BasicOrder_Createandtest_basicOrderRevertWhenConditionNotMet. For better readability and maintainability, consider following a consistent naming convention throughout the test suite.Also applies to: 193-193, 207-207, 226-226
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
solidity/orders/.prettierignore(2 hunks)solidity/orders/README.md(1 hunks)solidity/orders/package.json(1 hunks)solidity/orders/remappings.txt(1 hunks)solidity/orders/script/Deploy.s.sol(0 hunks)solidity/orders/src/BasicOrder.sol(1 hunks)solidity/orders/src/Foo.sol(0 hunks)solidity/orders/src/IExecution.sol(1 hunks)solidity/orders/src/OrderFactory.sol(1 hunks)solidity/orders/src/Types.sol(1 hunks)solidity/orders/test/BasicOrder.t.sol(1 hunks)solidity/orders/test/Foo.t.sol(0 hunks)solidity/orders/test/mocks/MockSlinkyPrecompile.sol(1 hunks)solidity/orders/test/mocks/MockWardenPrecompile.sol(1 hunks)
💤 Files with no reviewable changes (3)
- solidity/orders/script/Deploy.s.sol
- solidity/orders/src/Foo.sol
- solidity/orders/test/Foo.t.sol
🚧 Files skipped from review as they are similar to previous changes (7)
- solidity/orders/.prettierignore
- solidity/orders/package.json
- solidity/orders/remappings.txt
- solidity/orders/src/IExecution.sol
- solidity/orders/src/Types.sol
- solidity/orders/test/mocks/MockSlinkyPrecompile.sol
- solidity/orders/test/mocks/MockWardenPrecompile.sol
🧰 Additional context used
📓 Path-based instructions (1)
solidity/orders/README.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 LanguageTool
solidity/orders/README.md
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...p contracts ### Commands Before using commands install dependencies with `yarn install...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (6)
solidity/orders/README.md (1)
8-16: LGTM: Clear and consistent build/test instructions
The build and test instructions are clear, properly formatted in code blocks, and consistently use yarn commands.
solidity/orders/src/OrderFactory.sol (3)
1-8: LGTM! Well-structured imports and version constraints.
The file structure follows best practices with clear license, pragma, and organized imports.
33-44: LGTM! Well-implemented constructor with proper validations.
The constructor correctly validates addresses and initializes state variables.
46-61: 🛠️ Refactor suggestion
Consider removing scheduler parameter from createOrder
As suggested in past reviews, consider moving scheduler management entirely to the constructor and admin functions, rather than allowing it to be passed in createOrder. This would provide better control over scheduler changes.
function createOrder(
Types.OrderData calldata _orderData,
- CommonTypes.Coin[] calldata maxKeychainFees,
- OrderType orderType
+ CommonTypes.Coin[] calldata maxKeychainFees
)
public
returns (address order)
{
if (orderType == OrderType.Basic) {
- return _createBasicOrder(_orderData, maxKeychainFees, scheduler);
+ return _createBasicOrder(_orderData, maxKeychainFees);
} else if (orderType == OrderType.Advanced) {
return _createAdvancedOrder(_orderData, maxKeychainFees);
} else {
revert UnsupportedOrder();
}
}Likely invalid or redundant comment.
solidity/orders/src/BasicOrder.sol (2)
96-106: 🛠️ Refactor suggestion
Add validation for gas-related parameters
The execute function accepts gas-related parameters without proper validation, which could lead to failed transactions.
+ if (gas == 0) revert("Invalid gas value");
+ if (maxFeePerGas > 0 && gasPrice > 0) revert("Cannot specify both gasPrice and maxFeePerGas");
+ if (maxFeePerGas < maxPriorityFeePerGas) revert("MaxFeePerGas must be >= maxPriorityFeePerGas");Likely invalid or redundant comment.
71-76:
Add error handling for precompile contract calls
The constructor makes external calls to precompile contracts without proper error handling. These calls could fail and make contract deployment impossible.
- KeyResponse memory keyResponse = _wardenPrecompile.keyById(_orderData.signRequestData.keyId, new int32[](0));
+ try _wardenPrecompile.keyById(_orderData.signRequestData.keyId, new int32[](0)) returns (KeyResponse memory keyResponse) {
+ _keyAddress = address(bytes20(keccak256(keyResponse.key.publicKey)));
+ } catch {
+ revert("Failed to retrieve key from warden precompile");
+ }Likely invalid or redundant comment.
| for (uint256 i = 0; i < maxKeychainFees.length; i++) { | ||
| _coins.push(maxKeychainFees[i]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for maxKeychainFees array length
The loop that copies maxKeychainFees could potentially consume excessive gas if the array is too large. Consider adding a maximum length check.
+ if (maxKeychainFees.length > 10) { // adjust maximum as needed
+ revert("MaxKeychainFees array too long");
+ }
for (uint256 i = 0; i < maxKeychainFees.length; i++) {Committable suggestion skipped: line range outside the PR's diff.
| function _packSwapData() internal view returns (bytes memory data) { | ||
| data = abi.encodeWithSignature( | ||
| SWAP_EXACT_ETH_FOR_TOKENS, | ||
| orderData.swapData.amountIn, | ||
| orderData.swapData.path, | ||
| orderData.swapData.to, | ||
| orderData.swapData.deadline | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for swap path array
The _packSwapData function packs the path array without validating its contents. Consider adding checks for:
- Minimum path length (at least 2 tokens)
- Valid token addresses (non-zero)
- Path starts with wrapped native token for ETH swaps
function _packSwapData() internal view returns (bytes memory data) {
+ if (orderData.swapData.path.length < 2) {
+ revert("Invalid path length");
+ }
+ for (uint i = 0; i < orderData.swapData.path.length; i++) {
+ if (orderData.swapData.path[i] == address(0)) {
+ revert("Invalid token address in path");
+ }
+ }
data = abi.encodeWithSignature(Committable suggestion skipped: line range outside the PR's diff.
| condition = 0; | ||
| } | ||
|
|
||
| address[] memory path; |
There was a problem hiding this comment.
Validate that swapData.path is not empty
The swapData.path array is initialized but left empty. An empty path can lead to errors during the swap execution, as the swap function may require at least one token address in the path. Ensure that swapData.path is properly populated with the token addresses involved in the swap.
Apply this diff to initialize path with the necessary token addresses:
- address[] memory path;
+ address[] memory path = new address[](2);
+ // Initialize 'path' with token addresses
+ path[0] = address(/* Token A address */);
+ path[1] = address(/* Token B address */);Also applies to: 158-158, 246-246, 253-253
| function test_basicOrderExecuteWhenPriceMovesDown() public { | ||
| uint256 price = _testData.thresholdPrice - 1; | ||
| _testData.mockSlinkyPrecompile.setPrice(_testData.pricePair.base, _testData.pricePair.quote, price); | ||
|
|
||
| assert(_order.canExecute()); | ||
|
|
||
| vm.expectEmit(false, false, false, false); | ||
| emit Executed(); | ||
|
|
||
| bool executed = _order.execute(1, 1, 1, 1, 1); | ||
|
|
||
| assert(executed); | ||
|
|
||
| assert(_order.isExecuted()); | ||
|
|
||
| vm.expectRevert(ExecutedError.selector); | ||
| _order.execute(1, 1, 1, 1, 1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce code duplication in test functions
The test functions test_basicOrderExecuteWhenPriceMovesDown and test_basicOrderExecuteWhenPriceMovesUp contain similar logic with minor differences. Consider refactoring the shared code into a helper function to improve maintainability and reduce duplication.
Also applies to: 226-243
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
solidity/orders/src/OrderFactory.sol (1)
82-82: Address TODO comment about registry registrationThe TODO comment indicates missing functionality for registering the order in the registry. This should be implemented to ensure proper order tracking.
Would you like me to help implement the registry registration functionality or create a GitHub issue to track this task?
solidity/orders/src/BasicOrder.sol (1)
11-24: Add error definition for invalid swap pathConsider adding an error definition for invalid swap path length to maintain consistency with other validation errors.
+error InvalidSwapPath();solidity/orders/test/BasicOrder.t.sol (4)
207-243: Refactor test functions to reduce code duplicationThe test functions
test_basicOrderExecuteWhenPriceMovesDownandtest_basicOrderExecuteWhenPriceMovesUpcontain similar logic. Consider refactoring shared code into a helper function to improve maintainability.Example refactoring:
+ function executeOrderAtPrice(uint256 price) internal { + _testData.mockSlinkyPrecompile.setPrice(_testData.pricePair.base, _testData.pricePair.quote, price); + assert(_order.canExecute()); + vm.expectEmit(false, false, false, false); + emit Executed(); + bool executed = _order.execute(1, 1, 1, 1, 1); + assert(executed); + assert(_order.isExecuted()); + vm.expectRevert(ExecutedError.selector); + _order.execute(1, 1, 1, 1, 1); + } function test_basicOrderExecuteWhenPriceMovesDown() public { - uint256 price = _testData.thresholdPrice - 1; - _testData.mockSlinkyPrecompile.setPrice(_testData.pricePair.base, _testData.pricePair.quote, price); - - assert(_order.canExecute()); - - vm.expectEmit(false, false, false, false); - emit Executed(); - - bool executed = _order.execute(1, 1, 1, 1, 1); - - assert(executed); - - assert(_order.isExecuted()); - - vm.expectRevert(ExecutedError.selector); - _order.execute(1, 1, 1, 1, 1); + executeOrderAtPrice(_testData.thresholdPrice - 1); } function test_basicOrderExecuteWhenPriceMovesUp() public { - uint256 price = _testData.thresholdPrice + 1; - _testData.mockSlinkyPrecompile.setPrice(_testData.pricePair.base, _testData.pricePair.quote, price); - - assert(_order.canExecute()); - - vm.expectEmit(false, false, false, false); - emit Executed(); - - bool executed = _order.execute(1, 1, 1, 1, 1); - - assert(executed); - - assert(_order.isExecuted()); - - vm.expectRevert(ExecutedError.selector); - _order.execute(1, 1, 1, 1, 1); + executeOrderAtPrice(_testData.thresholdPrice + 1); }
52-54: Remove redundant error declarationThe error
OwnableUnauthorizedAccountis already defined in OpenZeppelin'sOwnable.sol. Since you're importing OpenZeppelin contracts, you can remove this redundant declaration to avoid conflicts.Apply this diff to remove the redundant error:
- // @openzeppelin Ownable.sol - error OwnableUnauthorizedAccount(address account);
55-57: Consider making addresses configurable for flexibilityHardcoding addresses like
SEPOLIA_UNISWAP_V2_ROUTERandRECEIVERcan limit testing flexibility across different environments. Consider making them configurable via constructor parameters or environment variables.Example adjustment:
- address private constant SEPOLIA_UNISWAP_V2_ROUTER = - address(bytes20(bytes("0xeE567Fe1712Faf6149d80dA1E6934E354124CfE3"))); - address private constant RECEIVER = address(bytes20(bytes("0x18517Cb2779186B86b1F8947dFdB6078C1B9C9db")); + address private immutable uniswapV2Router; + address private immutable receiver; + + constructor(address _uniswapV2Router, address _receiver) { + uniswapV2Router = _uniswapV2Router; + receiver = _receiver; + }
96-98: Use mock addresses for clarityUsing
address(this)forregistry,scheduler, andownerin thesetUpfunction may reduce readability. Consider defining explicit mock addresses to clarify their roles.Example:
- address registry = address(this); - address scheduler = address(this); - address owner = address(this); + address mockRegistry = address(this); + address mockScheduler = address(this); + address mockOwner = address(this);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
solidity/orders/src/BasicOrder.sol(1 hunks)solidity/orders/src/OrderFactory.sol(1 hunks)solidity/orders/test/BasicOrder.t.sol(1 hunks)
🔇 Additional comments (11)
solidity/orders/src/OrderFactory.sol (3)
33-44: LGTM! Well-structured constructor with proper validations
The constructor follows best practices:
- Validates zero addresses
- Properly initializes state variables
- Correctly passes owner to the Ownable constructor
88-97: LGTM! Well-structured unimplemented function
The _createAdvancedOrder function is properly marked as unimplemented:
- Correctly marked as
puresince it doesn't access state - Uses a custom error for unimplemented state
- Has proper parameter names omitted since they're unused
2-2: Consider relaxing the minimum Solidity version requirement
The pragma version >=0.8.25 is very recent and might limit compatibility with some development environments or tools. Consider using a more established version like >=0.8.20 unless specific 0.8.25 features are required.
solidity/orders/src/BasicOrder.sol (4)
1-10: LGTM! Well-structured imports and version pragma
The contract uses appropriate version constraints and necessary imports. The inclusion of ReentrancyGuard shows good security practices.
180-188: 🛠️ Refactor suggestion
Add path validation and documentation
The _packSwapData function should validate the swap path and include natspec documentation.
+ /// @notice Packs swap data for transaction execution
+ /// @return data The packed swap data
function _packSwapData() internal view returns (bytes memory data) {
+ if (orderData.swapData.path.length < 2) {
+ revert InvalidSwapPath();
+ }
+ for (uint i = 0; i < orderData.swapData.path.length; i++) {
+ if (orderData.swapData.path[i] == address(0)) {
+ revert("Invalid token address in path");
+ }
+ }Likely invalid or redundant comment.
38-41: 🛠️ Refactor suggestion
Optimize array copying and add length validation
The array copying loop could be gas-intensive for large arrays. Consider adding a maximum length check and using memory pointer optimization.
+ if (maxKeychainFees.length > 10) {
+ revert("MaxKeychainFees array too long");
+ }
+ assembly {
+ // Get the pointer to the free memory
+ let ptr := mload(0x40)
+ // Copy the array to memory
+ calldatacopy(ptr, maxKeychainFees.offset, mul(maxKeychainFees.length, 0x20))
+ // Update the free memory pointer
+ mstore(0x40, add(ptr, mul(maxKeychainFees.length, 0x20)))
+ }Likely invalid or redundant comment.
96-106:
Add validation for gas-related parameters
The execute function should validate gas-related parameters to prevent potential issues with transaction execution.
+ if (gas == 0) {
+ revert("Invalid gas value");
+ }
+ if (maxFeePerGas > 0 && gasPrice > 0) {
+ revert("Cannot specify both gasPrice and maxFeePerGas");
+ }
+ if (maxFeePerGas < maxPriorityFeePerGas) {
+ revert("Invalid fee values");
+ }Likely invalid or redundant comment.
solidity/orders/test/BasicOrder.t.sol (4)
133-133:
Initialize path to prevent potential issues
The path array is declared but not initialized. An empty path may cause issues when used in swaps. Ensure that path is properly initialized with the required token addresses.
Apply this diff to initialize path:
- address[] memory path;
+ address[] memory path = new address[](2);
+ path[0] = address(/* Token A address */);
+ path[1] = address(/* Token B address */);Likely invalid or redundant comment.
203-203:
Avoid using address(0) with vm.broadcast
Using address(0) as the sender in vm.broadcast is inappropriate and can lead to unexpected behavior. Use a valid address to simulate unauthorized access.
Apply this diff to use a valid unauthorized address:
- vm.broadcast(address(0));
+ address unauthorizedUser = address(0xDEADBEEF);
+ vm.broadcast(unauthorizedUser);Likely invalid or redundant comment.
129-131: 🛠️ Refactor suggestion
Enhance condition validation in test_BasicOrder_Create
The condition check if (condition > 1) silently defaults to 0 for invalid values. It's better to validate the input and revert if an invalid condition is provided to catch unintended misuse.
Apply this diff to improve the validation:
- if (condition > 1) {
- condition = 0;
- }
+ require(condition <= uint8(Types.PriceCondition.GTE), "Invalid condition");Likely invalid or redundant comment.
154-173: Validate uninitialized parameters in OrderData
Parameters like analyzers, encryptionKey, and path are declared but not initialized. Ensure that passing empty arrays or uninitialized values is acceptable, or initialize them appropriately to prevent unexpected behavior.
Run the following script to check for usage of uninitialized parameters:
✅ Verification successful
Empty arrays are acceptable for analyzers, encryptionKey, and path parameters
Based on the code analysis:
- The
OrderDatastruct definition inTypes.solshows these fields are part of the structure - The
BasicOrderconstructor only validatesswapData.amountInmust be non-zero and checks for a valid scheduler - No explicit validation exists requiring non-empty arrays for
analyzers,encryptionKey, orpath - The test file's usage of empty values appears to be intentional and valid
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for constructors or functions that require non-empty arrays.
# Find all instances where OrderData is used and check requirements.
rg 'OrderData\s*memory' -A 10 | rg 'require.*analyzers|require.*encryptionKey|require.*path'
Length of output: 94
Script:
#!/bin/bash
# Let's search for any validation or requirements on these fields in the contract code
# First, find all files that might contain OrderData struct definition and its usage
fd -e sol | xargs rg -l "OrderData"
# Then let's examine the struct definition and any validation logic
fd -e sol | xargs rg -A 5 "struct\s+OrderData"
# Also search for any validation in functions accepting OrderData
fd -e sol | xargs rg -B 2 -A 10 "function.*OrderData.*memory"
Length of output: 673
Script:
#!/bin/bash
# Let's check the BasicOrder contract for any validation of these fields
rg -B 2 -A 10 "constructor.*OrderData|function.*OrderData" solidity/orders/src/BasicOrder.sol
# Also check OrderFactory for any validation during order creation
rg -B 2 -A 10 "constructor.*OrderData|function.*OrderData" solidity/orders/src/OrderFactory.sol
# Let's also check if there are any require statements related to these fields
rg "require.*(\b(analyzers|encryptionKey|path)\b)" solidity/orders/src/
Length of output: 719
| function createOrder( | ||
| Types.OrderData calldata _orderData, | ||
| CommonTypes.Coin[] calldata maxKeychainFees, | ||
| OrderType orderType | ||
| ) | ||
| public | ||
| returns (address order) | ||
| { | ||
| if (orderType == OrderType.Basic) { | ||
| return _createBasicOrder(_orderData, maxKeychainFees, scheduler); | ||
| } else if (orderType == OrderType.Advanced) { | ||
| return _createAdvancedOrder(_orderData, maxKeychainFees); | ||
| } else { | ||
| revert UnsupportedOrder(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add reentrancy protection to createOrder
The createOrder function makes external contract calls through _createBasicOrder but doesn't follow the checks-effects-interactions pattern. Consider adding the nonReentrant modifier from OpenZeppelin's ReentrancyGuard.
Apply this diff:
-import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
+import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
+import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
-contract OrderFactory is Ownable {
+contract OrderFactory is Ownable, ReentrancyGuard {
function createOrder(
Types.OrderData calldata _orderData,
CommonTypes.Coin[] calldata maxKeychainFees,
OrderType orderType
)
public
+ nonReentrant
returns (address order)📝 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.
| function createOrder( | |
| Types.OrderData calldata _orderData, | |
| CommonTypes.Coin[] calldata maxKeychainFees, | |
| OrderType orderType | |
| ) | |
| public | |
| returns (address order) | |
| { | |
| if (orderType == OrderType.Basic) { | |
| return _createBasicOrder(_orderData, maxKeychainFees, scheduler); | |
| } else if (orderType == OrderType.Advanced) { | |
| return _createAdvancedOrder(_orderData, maxKeychainFees); | |
| } else { | |
| revert UnsupportedOrder(); | |
| } | |
| } | |
| import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; | |
| import { ReentrancyGuard } from "@openzeppelin/contracts/security/ReentrancyGuard.sol"; | |
| contract OrderFactory is Ownable, ReentrancyGuard { | |
| function createOrder( | |
| Types.OrderData calldata _orderData, | |
| CommonTypes.Coin[] calldata maxKeychainFees, | |
| OrderType orderType | |
| ) | |
| public | |
| nonReentrant | |
| returns (address order) | |
| { | |
| if (orderType == OrderType.Basic) { | |
| return _createBasicOrder(_orderData, maxKeychainFees, scheduler); | |
| } else if (orderType == OrderType.Advanced) { | |
| return _createAdvancedOrder(_orderData, maxKeychainFees); | |
| } else { | |
| revert UnsupportedOrder(); | |
| } | |
| } |
| WARDEN_PRECOMPILE = IWarden(IWARDEN_PRECOMPILE_ADDRESS); | ||
| KeyResponse memory keyResponse = WARDEN_PRECOMPILE.keyById(_orderData.signRequestData.keyId, new int32[](0)); | ||
| _keyAddress = address(bytes20(keccak256(keyResponse.key.publicKey))); | ||
|
|
||
| SLINKY_PRECOMPILE = ISlinky(ISLINKY_PRECOMPILE_ADDRESS); | ||
| SLINKY_PRECOMPILE.getPrice(_orderData.pricePair.base, _orderData.pricePair.quote); | ||
|
|
There was a problem hiding this comment.
Add error handling for precompile calls
The calls to WARDEN_PRECOMPILE and SLINKY_PRECOMPILE could fail. Consider adding try-catch blocks.
- KeyResponse memory keyResponse = WARDEN_PRECOMPILE.keyById(_orderData.signRequestData.keyId, new int32[](0));
+ try WARDEN_PRECOMPILE.keyById(_orderData.signRequestData.keyId, new int32[](0)) returns (KeyResponse memory keyResponse) {
+ _keyAddress = address(bytes20(keccak256(keyResponse.key.publicKey)));
+ } catch {
+ revert("Failed to retrieve key");
+ }Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
New Features
BasicOrdersmart contract for managing order execution based on price conditions.OrderFactorycontract to facilitate the creation and management of order contracts.MockSlinkyPrecompileandMockWardenPrecompilecontracts for testing purposes.Improvements
Bug Fixes
Tests
BasicOrdercontract to ensure proper functionality and state validation.