test: drop duplicate MockAddressRevertingFactory import - #123
Conversation
`test/src/lib/LibRainDeploy.t.sol` imported `MockAddressRevertingFactory` from the same path at lines 9 and 12, byte-identically. solc tolerates a repeated identical declaration, so it compiled and nothing failed, which is why it survived — the cost was a reader having to diff two lines to conclude they are the same. The symbol is used, so this is a redundant duplicate line rather than a dead import. A repo-wide scan of imported symbols per file finds this as the only case. Closes #71 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Reviewed 9c4eab2: ready — Closes #71. Read the diff. One line removed: CI green (rainix static/legal/test), 0 unresolved threads — qualified, since the CodeRabbit check reports |
1 participant
Closes #71
test/src/lib/LibRainDeploy.t.solimportedMockAddressRevertingFactoryfrom../../concrete/MockAddressRevertingFactory.solat line 9 and again, byte-identically, at line 12. This deletes the line-12 copy. One line, nothing else in the diff.The symbol is still used —
new MockAddressRevertingFactory()at what are now lines 400 and 1142 — so this removes a redundant duplicate line, not a live import. solc tolerates a repeated identical declaration, so nothing about compilation, runtime or behaviour changes. The whole cost was a reader having to diff two lines to conclude they are the same, which is also why it survived: nothing fails, so nothing points at it.Category, not just the cited line
The issue names one file and one symbol. The rule behind it is one import per symbol per file, so I checked that property across the repo rather than only the two cited lines: over every
*.soltracked in git, flattening each import's braced symbol list (multi-line import blocks included, so a symbol split across lines is still counted) and looking for any symbol imported twice in the same file returns exactly one hit — this file, this symbol. There is nothing else in the category, so the one-line fix is the whole fix.Why nothing caught it
nix develop -c forge fmt --checkexits 0 on the tree with the duplicate present, so the formatter neither flags nor removes it;slither.config.jsonis not configured for it; solc is silent by design. None of the three CI matrix tasks would have failed on this. This PR does not add a guard — that is scope beyond the issue — but the gap is worth stating rather than leaving implied.QA
LibRainDeployTeststill compiles and still exercisesMockAddressRevertingFactoryat both call sites.test/src/lib/LibRainDeploy.t.solline 9 — delete the surviving import as well, so the file has zero imports ofMockAddressRevertingFactoryagainst two usages -> killed bynix develop -c forge build, which fails withError (7920): Identifier not found or not unique.pointing at line 400. That is the mutation that matters for this change: it proves the line the PR keeps is load-bearing and the line the PR deletes is not, which is exactly the claim "this was a duplicate, not a dead import". The inverse direction is already established and is why the issue exists — restoring the duplicate leaves the build, the formatter and slither all green, so no mutant on the deleted line can be killed by anything.MockAddressRevertingFactoryresolves at lines 400 and 1142, and it is not consulted about which of two identical import lines provided it. The duplicate's own cited location came from the issue and was re-read against source before editing.Test runs.
nix develop -c forge fmt --checkexits 0. Fullnix develop -c forge testwith the five documented public RPC endpoints in.env: 202 passed across 17 suites, 16 suites fully green. The 13 failures were confined toLibRainDeploy.t.sol, and every one of the 26[FAIL]lines is an HTTP 429 public-RPC rate limit (Public RPC Rate Limit Hit/over rate limit) from the shared free endpoints — zero assertion failures, and no failure mentions the import, the mock, or anything this diff touches.Re-running that one suite with
--threads 1takes it to 53 passed / 1 failed of 54, the single remaining failure again a 429 fromarb1.arbitrum.io. Failures fall 13 -> 1 purely by lowering RPC concurrency, with no source change between the two runs, which is what identifies rate limiting rather than the diff as the cause. CI runs with real RPC secrets and is the verdict that counts.