Update fork tests to use mainnet addresses#1472
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughFork test setup across multiple modules was changed to use Ethereum mainnet-deployed contracts and factory methods. Fork.t.sol files now assign factory and core contracts from hardcoded mainnet addresses, remove local deployments and certain reinitialization steps, and update tests to compute expected Merkle campaign addresses via factory.computeMerkle*(campaignCreator, constructorParams). Some tests also use runtime-calculated fees and expanded protocol address lists. Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@utils/tests/fork/Comptroller.t.sol`:
- Line 66: Update the checklist comment above the test function
testForkFuzz_CalculateMinFeeWei to include "Bob" so it matches the assertion
string ("Airdrops, Bob, Flow, Lockup") — locate the comment block immediately
preceding testForkFuzz_CalculateMinFeeWei and edit the text from "Lockup, Flow
and Airdrops" to "Airdrops, Bob, Flow and Lockup" (or equivalent ordering
matching assertGt's message) so the checklist and the assertGt message are
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77a1fea1-885b-476d-baec-7f6998dbc0bb
📒 Files selected for processing (11)
airdrops/tests/fork/Fork.t.solairdrops/tests/fork/merkle-campaign/MerkleExecute.t.solairdrops/tests/fork/merkle-campaign/MerkleInstant.t.solairdrops/tests/fork/merkle-campaign/MerkleLL.t.solairdrops/tests/fork/merkle-campaign/MerkleLT.t.solairdrops/tests/fork/merkle-campaign/MerkleVCA.t.solbob/tests/bob/fork/Fork.t.solflow/tests/fork/Fork.t.sollockup/tests/fork/Fork.t.sollockup/tests/fork/LockupPriceGated.t.solutils/tests/fork/Comptroller.t.sol
andreivladbrg
left a comment
There was a problem hiding this comment.
generally looks good, two questions:
wdyt about adding the contract addresses in utils' test constants?
this way we'd have them only in one place
It would increase maintenance cost. With every change, we would have to bump utils. So I am not in favour of that approach. |
andreivladbrg
left a comment
There was a problem hiding this comment.
It would increase maintenance cost. With every change, we would have to bump utils. So I am not in favour of that approach.
yeah, fair enough lets keep it this way
Updated fork tests to use the mainnet contracts. Also closes #1335