Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(contracts-rfq): limit the amount of solhint warnings [SLT-245] #3182

Merged
merged 3 commits into from
Sep 25, 2024
Merged

fix(contracts-rfq): limit the amount of solhint warnings [SLT-245] #3182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e1bfa2
ci: limit the amount of solhint warnings
ChiTimesChi Sep 25, 2024
e462eff
refactor: move the errors into the separate interface
ChiTimesChi Sep 25, 2024
4796b36
refactor: errors imports in tests
ChiTimesChi Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/contracts-rfq/contracts/FastBridgeV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ pragma solidity 0.8.24;

import {SafeERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

import "./libs/Errors.sol";
import {UniversalTokenLib} from "./libs/UniversalToken.sol";

import {Admin} from "./Admin.sol";
import {IFastBridge} from "./interfaces/IFastBridge.sol";
import {IFastBridgeV2} from "./interfaces/IFastBridgeV2.sol";
import {IFastBridgeV2Errors} from "./interfaces/IFastBridgeV2Errors.sol";

contract FastBridgeV2 is Admin, IFastBridgeV2 {
contract FastBridgeV2 is Admin, IFastBridgeV2, IFastBridgeV2Errors {
using SafeERC20 for IERC20;
using UniversalTokenLib for address;

Expand Down
19 changes: 19 additions & 0 deletions packages/contracts-rfq/contracts/interfaces/IFastBridgeV2Errors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

interface IFastBridgeV2Errors {
error AmountIncorrect();
error ChainIncorrect();
error MsgValueIncorrect();
error SenderIncorrect();
error StatusIncorrect();
error ZeroAddress();

error DeadlineExceeded();
error DeadlineNotExceeded();
error DeadlineTooShort();
error DisputePeriodNotPassed();
error DisputePeriodPassed();

error TransactionRelayed();
Comment on lines +5 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding parameters to some errors for more detailed reporting.

The error definitions are clear and well-organized. However, some errors could benefit from additional context. For example:

error AmountIncorrect(uint256 expected, uint256 actual);
error ChainIncorrect(uint256 expected, uint256 actual);
error DeadlineExceeded(uint256 deadline, uint256 currentTime);

This would provide more specific information when these errors occur, aiding in debugging and improving the developer experience.

}
4 changes: 2 additions & 2 deletions packages/contracts-rfq/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"lint:check": "forge fmt --check && npm run solhint:check",
"ci:lint": "npm run lint:check",
"build:go": "./flatten.sh contracts/*.sol test/*.sol",
"solhint": "solhint '{contracts,script,test}/**/*.sol' --fix --noPrompt",
"solhint:check": "solhint '{contracts,script,test}/**/*.sol'"
"solhint": "solhint '{contracts,script,test}/**/*.sol' --fix --noPrompt --max-warnings 3",
"solhint:check": "solhint '{contracts,script,test}/**/*.sol' --max-warnings 3"
}
}
2 changes: 0 additions & 2 deletions packages/contracts-rfq/test/FastBridgeV2.Dst.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {ChainIncorrect, DeadlineExceeded, TransactionRelayed} from "../contracts/libs/Errors.sol";

import {FastBridgeV2, FastBridgeV2Test, IFastBridge} from "./FastBridgeV2.t.sol";

// solhint-disable func-name-mixedcase, ordering
Expand Down
6 changes: 4 additions & 2 deletions packages/contracts-rfq/test/FastBridgeV2.Parity.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import {FastBridgeTest, SenderIncorrect} from "./FastBridge.t.sol";
import {IFastBridgeV2Errors} from "../contracts/interfaces/IFastBridgeV2Errors.sol";

import {FastBridgeTest} from "./FastBridge.t.sol";

// solhint-disable func-name-mixedcase, ordering
contract FastBridgeV2ParityTest is FastBridgeTest {
contract FastBridgeV2ParityTest is FastBridgeTest, IFastBridgeV2Errors {
address public anotherRelayer = makeAddr("Another Relayer");

function deployFastBridge() internal virtual override returns (address) {
Expand Down
13 changes: 0 additions & 13 deletions packages/contracts-rfq/test/FastBridgeV2.Src.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,6 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.20;

import {
AmountIncorrect,
ChainIncorrect,
DisputePeriodNotPassed,
DisputePeriodPassed,
DeadlineNotExceeded,
DeadlineTooShort,
MsgValueIncorrect,
SenderIncorrect,
StatusIncorrect,
ZeroAddress
} from "../contracts/libs/Errors.sol";

import {FastBridgeV2, FastBridgeV2Test, IFastBridge} from "./FastBridgeV2.t.sol";

// solhint-disable func-name-mixedcase, ordering
Expand Down
3 changes: 2 additions & 1 deletion packages/contracts-rfq/test/FastBridgeV2.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
pragma solidity ^0.8.20;

import {IFastBridge} from "../contracts/interfaces/IFastBridge.sol";
import {IFastBridgeV2Errors} from "../contracts/interfaces/IFastBridgeV2Errors.sol";
import {FastBridgeV2} from "../contracts/FastBridgeV2.sol";

import {MockERC20} from "./MockERC20.sol";
Expand All @@ -11,7 +12,7 @@ import {Test} from "forge-std/Test.sol";
import {stdStorage, StdStorage} from "forge-std/Test.sol";

// solhint-disable no-empty-blocks, ordering
abstract contract FastBridgeV2Test is Test {
abstract contract FastBridgeV2Test is Test, IFastBridgeV2Errors {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Unused IFastBridgeV2Errors in FastBridgeV2Test

No instances of IFastBridgeV2Errors usage found within FastBridgeV2.t.sol beyond its inclusion in the contract inheritance and import statements.

Consider removing IFastBridgeV2Errors from the inheritance if it is not utilized, or ensure that it is appropriately used to enhance error handling in the test cases.

🔗 Analysis chain

LGTM: Contract signature updated to include IFastBridgeV2Errors.

The FastBridgeV2Test contract now inherits from both Test and IFastBridgeV2Errors, which is consistent with the new import statement. This change should improve error handling and testing capabilities, aligning with the PR's objective of enhancing code quality.

To ensure this change doesn't introduce any unintended side effects, please verify:

  1. All error types defined in IFastBridgeV2Errors are properly used in the test cases.
  2. There are no naming conflicts between Test and IFastBridgeV2Errors.

You can use the following script to check for the usage of error types:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for the usage of error types from IFastBridgeV2Errors in the test file

# Test: Search for error types usage
rg --type solidity -e "revert.*IFastBridgeV2Errors" packages/contracts-rfq/test/FastBridgeV2.t.sol

Length of output: 133

Script:

#!/bin/bash
# Description: Check for the usage of error types from IFastBridgeV2Errors in Solidity test files

# Test: Search for error types usage in .sol files
rg -g "*.sol" -e "revert.*IFastBridgeV2Errors" packages/contracts-rfq/test/FastBridgeV2.t.sol

Length of output: 95

Script:

#!/bin/bash
# Description: Search for any usage of IFastBridgeV2Errors in the Solidity test file

# Test: Find all references to IFastBridgeV2Errors
rg -g "*.sol" "IFastBridgeV2Errors" packages/contracts-rfq/test/FastBridgeV2.t.sol

Length of output: 233

using stdStorage for StdStorage;

uint32 public constant SRC_CHAIN_ID = 1337;
Expand Down
Loading