Skip to content

chore(common): enable sol typos checks#1941

Closed
Eikix wants to merge 3 commits intomainfrom
codex/issue-1909-sol-typos
Closed

chore(common): enable sol typos checks#1941
Eikix wants to merge 3 commits intomainfrom
codex/issue-1909-sol-typos

Conversation

@Eikix
Copy link
Copy Markdown
Contributor

@Eikix Eikix commented Feb 9, 2026

Summary

  • remove *.sol from typos.toml exclude list
  • fix Solidity typos reported by typos across contracts, examples, and test contracts

Testing

  • typos

Closes #1909

@Eikix Eikix requested review from a team as code owners February 9, 2026 07:56
@cla-bot cla-bot Bot added the cla-signed label Feb 9, 2026
@mergify
Copy link
Copy Markdown

mergify Bot commented Feb 9, 2026

🧪 CI Insights

Here's what we observed from your CI run for 4a09bd0.

🟢 All jobs passed!

But CI Insights is watching 👀

@Eikix
Copy link
Copy Markdown
Contributor Author

Eikix commented Feb 9, 2026

Quick follow-up on this PR: I excluded the upgrade-sensitive host contracts from typo checks for now.

Excluded paths:

  • host-contracts/contracts/ACL.sol
  • host-contracts/contracts/FHEVMExecutor.sol
  • host-contracts/contracts/InputVerifier.sol
  • host-contracts/contracts/KMSVerifier.sol
  • host-contracts/contracts/HCULimit.sol

Reason: these are deployment/upgrade-critical and wired in infra flows, so we should avoid typo-only edits there until infra confirms it’s safe.

If infra gives the green light, we can re-enable typo checks on these files.

@claude
Copy link
Copy Markdown

claude Bot commented Feb 10, 2026

Claude encountered an error —— View job


I'll analyze this and get back to you.

@Eikix
Copy link
Copy Markdown
Contributor Author

Eikix commented Feb 10, 2026

@claude /pr-review is it a risk that we are modifying contracts that are imported by critical contracts? can we perhaps change some sensitive compilation checksum?

@claude
Copy link
Copy Markdown

claude Bot commented Feb 10, 2026

Claude finished @Eikix's task in 1m 39s —— View job


PR Review - Risk Assessment for Typo Fixes on Critical Contracts

  • Gather context on changed files and their dependencies
  • Analyze which contracts are upgrade-sensitive
  • Check if modifications affect bytecode/checksums
  • Provide risk assessment

Summary

Yes, this is a risk. The PR modifies host-contracts/contracts/ACL.sol which is one of the upgrade-sensitive contracts that was supposed to be excluded.

The Inconsistency

Looking at the commit history:

  1. Commit 1 (821bfa9e): Fixed typos across all .sol files, including ACL.sol
  2. Commit 2 (b18f1b9c): Added exclusions to typos.toml for sensitive contracts

The problem: The exclusion was added after the typo was already fixed in ACL.sol. The exclusion only prevents future typo checks—it doesn't revert the change already made.

Files at Risk

The PR modifies host-contracts/contracts/ACL.sol (line 527: wetherwhether), which is:

  • An upgradeable proxy contract (uses OpenZeppelin's upgradeable pattern)
  • Listed as upgrade-sensitive in the PR author's own comment
  • Subject to task:upgradeACL Hardhat upgrade task
  • Potentially tracked in Helm charts for production deployments

Impact on Bytecode/Checksums

Changes to NatSpec comments (@notice, @param, etc.) do affect:

  1. Contract metadata (CBOR-encoded at end of bytecode by default)
  2. Compilation artifact checksums
  3. OpenZeppelin upgrades plugin validation (compares artifacts during upgrade)

This means:

  • Infra flows that verify bytecode checksums will see a different hash
  • Etherscan verification against existing deployments will fail
  • Any tooling comparing expected vs actual bytecode will flag this

Recommendation

Revert the ACL.sol change to be consistent with the stated intent of excluding upgrade-sensitive contracts:

git checkout origin/main -- host-contracts/contracts/ACL.sol

Or, if the team decides the typo fix is acceptable, remove ACL.sol from the exclusion list in typos.toml to avoid confusion.

Other Modified Contracts - Lower Risk

The other contracts modified are:

  • gateway-contracts/contracts/interfaces/IPauserSet.sol - Interface only (no deployment)
  • host-contracts/contracts/interfaces/IPauserSet.sol - Interface only (no deployment)
  • host-contracts/lib/Impl.sol - Library (may affect importing contracts)
  • host-contracts/examples/EncryptedERC20.sol - Example contract (not deployed to production)
  • library-solidity/ files - SDK library (not directly deployed)
  • test-suite/ files - Test contracts only

host-contracts/lib/Impl.sol deserves attention—it contains an interface IACL that might be imported by other contracts. Changes to interface comments could propagate.


@Eikix
Copy link
Copy Markdown
Contributor Author

Eikix commented Feb 10, 2026

Too risky to change sol files^^

@Eikix Eikix closed this Feb 10, 2026
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.

Remove typos ignore for .sol and fix typos

1 participant