Skip to content

Fix lint warnings#1741

Open
yrong wants to merge 15 commits intomainfrom
ron/lint-fix
Open

Fix lint warnings#1741
yrong wants to merge 15 commits intomainfrom
ron/lint-fix

Conversation

@yrong
Copy link
Contributor

@yrong yrong commented Mar 8, 2026

This PR is primarily a lint remediation and code-style consistency sweep across contracts, scripts, and tests.

  • Standardizes imports (named imports), removes unused imports/symbols, and cleans test/script files.
  • Rewrites many struct constructions to named-field syntax for clarity and lint compliance.
  • Adds explicit require(...) checks around ERC20 transfer/transferFrom calls in tests/scripts.
  • Refactors access-control modifiers in Gateway and Token into helper checks (_onlySelf, _onlyGateway) with no
    functional behavior change.
  • Adds targeted forge-lint suppression comments where casts/shifts are intentional.
  • Updates test env default for Fiat-Shamir required signatures to 111.

@codecov
Copy link

codecov bot commented Mar 8, 2026

Codecov Report

❌ Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.90%. Comparing base (f404d3d) to head (7fb71f3).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
contracts/src/Verification.sol 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1741      +/-   ##
==========================================
+ Coverage   76.60%   76.90%   +0.29%     
==========================================
  Files          24       24              
  Lines         979      983       +4     
  Branches      186      186              
==========================================
+ Hits          750      756       +6     
+ Misses        205      203       -2     
  Partials       24       24              
Flag Coverage Δ
solidity 76.90% <83.33%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yrong yrong marked this pull request as ready for review March 9, 2026 03:49
Comment on lines +28 to +37
[lint]
exclude_lints = [
"mixed-case-function",
"mixed-case-variable",
"asm-keccak256",
"unsafe-cheatcode",
"screaming-snake-case-immutable",
"screaming-snake-case-const",
"pascal-case-struct",
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The remaining excluded lint rules mainly relate to case and naming conventions.

In my opinion, fixing them could potentially break the public API and affect downstream consumers (e.g., the UI and relayers). The risk of introducing bugs during refactoring likely outweighs the benefit of strictly following Solidity naming conventions.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR primarily targets Solidity/Foundry lint cleanup across contracts, tests, and scripts by removing unused imports, adjusting ERC20 call sites, and adding lint suppressions/config.

Changes:

  • Removes/normalizes unused or redundant imports across contracts/tests/scripts; adds Foundry lint configuration and targeted forge-lint suppressions.
  • Updates many ERC20 transfer / transferFrom call sites to explicitly check return values (or annotates exceptions).
  • Refactors a couple of modifiers (onlyGateway, onlySelf) to delegate checks into internal helper functions.

Reviewed changes

Copilot reviewed 53 out of 53 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
web/packages/test/scripts/set-env.sh Updates default env var for Fiat-Shamir required signatures.
contracts/test/utils/ForkTestFixtures.sol Removes unused imports from fork test fixtures.
contracts/test/mocks/ReantrantAttacker.sol Removes unused console import.
contracts/test/mocks/MockGateway.sol Cleans imports; adjusts Functions import location.
contracts/test/mocks/BeefyClientMock.sol Removes unused imports; keeps only needed helper import.
contracts/test/Verification.t.sol Cleans test imports to reduce lint warnings.
contracts/test/Uint16Array.t.sol Normalizes imports; keeps console usage explicit.
contracts/test/Token.t.sol Adds checked ERC20 transfers / lint suppressions where appropriate; removes unused imports.
contracts/test/SparseBitmap.t.sol Removes unused imports.
contracts/test/ScaleCodec.t.sol Removes unused imports.
contracts/test/Math.t.sol Normalizes imports; converts struct literals to named-field syntax.
contracts/test/MMRProof.t.sol Removes unused console import.
contracts/test/GatewayV2.t.sol Cleans imports and updates some struct literals; (currently contains a duplicate import).
contracts/test/GatewayV1.t.sol Cleans imports; updates struct literals to named-field syntax; checks ERC20 transfers.
contracts/test/FiatShamirPaddingBitsGrind.t.sol Normalizes imports; adds lint suppressions for casts.
contracts/test/Bitfield.t.sol Removes unused console import.
contracts/test/BeefyClientAdvanced.t.sol Normalizes imports; adds lint suppressions for casts/tuple typing.
contracts/test/BeefyClient.t.sol Normalizes imports; adds lint suppressions for casts.
contracts/test/Agent.t.sol Removes unused imports.
contracts/src/v2/Handlers.sol Removes unused import.
contracts/src/v2/Calls.sol Removes unused PRBMath import.
contracts/src/v1/Types.sol Adds lint suppressions on imports (re-export intent).
contracts/src/v1/Handlers.sol Removes unused import.
contracts/src/v1/Calls.sol Removes unused imports; narrows PRBMath imports.
contracts/src/utils/Uint16Array.sol Adjusts masking/cast pattern; adds lint suppression.
contracts/src/utils/SparseBitmap.sol Adds lint suppression for shift warning.
contracts/src/utils/ScaleCodec.sol Adds lint suppressions around intentional casts.
contracts/src/utils/Math.sol Adds lint suppression for shift warning.
contracts/src/utils/Bitfield.sol Adds lint suppressions around intentional casts.
contracts/src/upgrade/Gateway202603.sepolia.sol Switches to named imports; adds ERC1967 usage for init guard.
contracts/src/upgrade/Gateway202602.sol Switches to named imports; adds ERC1967 usage for init guard.
contracts/src/upgrade/Gateway202602.sepolia.sol Switches to named imports; adds ERC1967 usage for init guard.
contracts/src/upgrade/Gateway202509.sol Switches to named imports; adds ERC1967 and AssetsStorage import usage.
contracts/src/types/Common.sol Removes unused PRBMath import.
contracts/src/l2-integration/SnowbridgeL1Adaptor.sol Removes unused imports.
contracts/src/interfaces/IERC20Metadata.sol Switches to named import for IERC20.
contracts/src/Verification.sol Removes unused import; adds lint suppressions for cast-to-bytes1 cases.
contracts/src/Types.sol Re-export hub updated with extensive unused-import suppressions.
contracts/src/Token.sol Refactors onlyGateway modifier check into internal helper.
contracts/src/Initializer.sol Removes unused imports; narrows PRBMath imports; adds lint suppression for re-exported types.
contracts/src/Gateway.sol Removes unused imports; refactors onlySelf modifier check into internal helper; narrows PRBMath imports.
contracts/src/BeefyClient.sol Adds lint suppression for bytes2 literal cast.
contracts/scripts/upgrade/DeployGateway.sol Cleans imports/order to resolve lint warnings.
contracts/scripts/l2-integration/across/test/TestUniswapQuoter.s.sol Cleans imports to resolve lint warnings.
contracts/scripts/l2-integration/across/test/TestUniswap.s.sol Cleans imports; restores explicit IERC20 import.
contracts/scripts/l2-integration/across/test/TestSnowbridgeL2AdaptorWeth.s.sol Cleans imports to resolve lint warnings.
contracts/scripts/l2-integration/across/test/TestSnowbridgeL2AdaptorNativeEther.s.sol Cleans imports to resolve lint warnings.
contracts/scripts/l2-integration/across/test/TestSnowbridgeL2Adaptor.s.sol Cleans imports to resolve lint warnings.
contracts/scripts/l2-integration/across/test/TestSnowbridgeL1AdaptorNativeEther.s.sol Cleans imports to resolve lint warnings.
contracts/scripts/l2-integration/across/test/TestSnowbridgeL1Adaptor.s.sol Cleans imports; checks IERC20 transfer return value.
contracts/scripts/l2-integration/across/deploy/DeploySnowbridgeL2Adaptor.s.sol Cleans imports; removes console logging.
contracts/scripts/DeployLocal.sol Narrows PRBMath import; checks WETH transfer return value.
contracts/foundry.toml Adds lint configuration and expands ignored error codes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

export RANDAO_COMMIT_EXP="${ETH_RANDAO_EXP:-32}"
export MINIMUM_REQUIRED_SIGNATURES="${MINIMUM_REQUIRED_SIGNATURES:-17}"
export FIAT_SHAMIR_REQUIRED_SIGNATURES="${FIAT_SHAMIR_REQUIRED_SIGNATURES:-101}"
export FIAT_SHAMIR_REQUIRED_SIGNATURES="${FIAT_SHAMIR_REQUIRED_SIGNATURES:-111}"
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The default for FIAT_SHAMIR_REQUIRED_SIGNATURES changes from 101 to 111, which is a behavioral change rather than a lint-only change. If this is intentional, it should be explained in the PR (or in a comment in this script); otherwise, revert to the previous default to avoid unexpected test/deploy configuration changes.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a fix, as the proof fixtures are actually generated using the 111 sample.

yrong and others added 2 commits March 9, 2026 12:16
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
pragma solidity 0.8.33;

// Re-export imports for public API
// forge-lint: disable-next-line(unused-import)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can disable an entire block which is neater: https://www.getfoundry.sh/forge/linting#disable-a-block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yrong yrong requested a review from vgeddes March 12, 2026 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants