Fix CI (forge-std), refresh docs, enforce fmt, assert ERC20 returns - #9
Merged
Conversation
The README still described three contracts and 13 tests. The repo now has seven contracts and 51 tests, and the invariant handler lives in test/VaultInvariant.t.sol, not the VaultHandler.sol the README referenced. Update the intro, the contract table and the handler note to match.
Add a 'forge fmt --check' step and format the sources it flagged, so style stays consistent. Change the gas step from 'forge snapshot' (which just regenerated the file and discarded it) to 'forge snapshot --check', so a gas regression against the committed .gas-snapshot fails the build.
The Token transfer/transferFrom calls on the success paths ignored the bool return value, which forge lint flags (erc20-unchecked-transfer) and which left the documented 'returns (bool)' behaviour untested. Assert the return where the call is expected to succeed. Regenerate the gas snapshot accordingly.
lib/ was gitignored and forge-std was vendored as plain (untracked) files, so CI had no forge-std to build against and every run failed at 'forge build' with 'forge-std/Test.sol not found'. Track forge-std as a submodule pinned to v1.16.1 (the version already in use, so gas and tests are unchanged) and stop ignoring lib/, so the existing 'submodules: recursive' checkout provides it.
The gas snapshot check failed in CI because fuzz-test gas depends on the random inputs, which differ per run. Pin the fuzz seed and the evm version so the gas figures are a deterministic function of the (already pinned) solc bytecode and the inputs, which makes 'forge snapshot --check' stable across runs and CI.
Fuzz-test gas depends on the fuzzer's random inputs, and the input sequence for a given seed differs across foundry versions, so 'forge snapshot --check' fails in CI even with a pinned seed unless the exact foundry version is pinned too. That is too brittle to be worth it here. Run 'forge snapshot' for a gas report in the logs without failing the build, and drop the seed pin so the fuzzer explores different inputs across runs again. The evm version stays pinned.
oliv3rdrt
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A batch of small improvements. Notably, CI had been failing on every run (including main):
lib/was gitignored and forge-std was vendored as untracked files, so CI had no forge-std and everyforge buildfailed. This is now green.fix: track forge-std as a submodule. Stop gitignoring
lib/and pin forge-std as a submodule at v1.16.1 (the version already in use, so gas and tests are unchanged). The existingsubmodules: recursivecheckout now provides it, so CI builds for the first time.docs: refresh the README. It still described three contracts and 13 tests; the repo now has seven contracts and 51 tests, and the invariant handler lives in
test/VaultInvariant.t.sol(the referencedVaultHandler.solnever existed). Updated the intro, the contract table, and the handler note.ci: enforce formatting. Added a
forge fmt --checkstep and formatted the three files it flagged. The gas step stays informational (forge snapshot): fuzz-test gas depends on the fuzzer's random inputs, whose sequence differs across foundry versions, so--checkcan't be enforced reliably without also pinning the exact foundry version. Theevm_versionis now pinned explicitly.test: assert ERC20 transfer return values. The success-path
transfer/transferFromcalls inToken.t.solignored the returnedbool(flagged byforge lint), leaving the documentedreturns (bool)behaviour untested. Wrapped those inassertTrue. The threevm.expectRevertcalls are left as is, since a reverting call has no return value to check.Verified locally and in CI:
forge fmt --checkclean,forge buildOK,forge test51 passed.