-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: improve bound
so there's no shifting of input values when they're within range
#188
Comments
The proposed solution suffices for arbitrary values, but it won't help with edge conditions, particularly on the upper range. E.g. if I bound an 8 bit value to [10, 20], input value 255 is mapped to |
Yep that's true, this mainly fixes dict limitations and ensures that stays useful, but doesn't help with edge values. One thing implicit in #1350 (which I'll add a comment for) is that it would also require a change to the edge-biasing strategy to factor in the user-provided upper limit |
This has also been a pain point in many of my tests. In particular, I often find myself writing general fuzz unit-tests (that use bound) and compose these in other tests. Then when dealing with bound, I always need to take the shift into account which gets messy. This is an invariant that should hold imo:
Some other things to note about
|
Just pushed a commit to #184 that should improve
This is true, though you could also have a range like [2^128, 2^256]. Using an inclusive range makes bounds easier to work with IMO, even though it's not necessary common for slice/range type methods
You're saying when you call a fuzz test from within a concrete (non-fuzz) test, right? i.e. adding a |
Ah actually I don't think I'm handling the Edit: nevermind, it's currently correct actually. Because |
Looks good to me.
Yes, having the logs is very helpful for failing fuzz tests, but makes it ugly to combine with extended unit-tests / user-journey tests. Though you're right, it should be tracked in a separate issue. |
* Modularize forge-std (#126) * refactor: unbundle cheats from assertions refactor: new category StdUtils refactor: unbundle Test from Script * Rename "vm" to "vm_cheats" in "Cheats.sol" Mark "vm_cheats" as "private" Instantiate a "vm" in "Test.sol" * refactor: remove deprecated "lowLevelError" refactor: rename "vm_cheats" to just "vm" refactor: rename "vm_std_store" to just "vm" refactor: delete "INT256_MAX" and "UINT256_MAX" revert: redeclare "stdstore" in "Test" * refactor: move "stdErrors" to "Errors.sol" refactor: move "stdMath" to "Math.sol" * Add note about versions in "Errors.sol| Co-authored-by: Zero Ekkusu <94782988+ZeroEkkusu@users.noreply.github.com> * chore: delete stale delineators in Errors and Math chore: delete stale "using stdStorage for StdStorage" * refactor: modularize assertions and utils docs: add NatSpec tag @dev in "console2" refactor: delete log from "bound" function refactor: move "addressFromLast20Bytes" to "Utils.sol" refactor: move "bound" to "Utils.sol" refactor: move "computeCreateAddress" to "Utils.sol" style: move brackets on same line with "if" and "else" in "bound" * Log bound result with static call to `console.log` Co-authored-by: Zero Ekkusu <94782988+ZeroEkkusu@users.noreply.github.com> * fix: reintroduce "vm" in "Script.sol" chore: silence compiler warning in "bound" refactor: define console2.log address as constant in "Utils.sol" * test: move "testGenerateCorrectAddress" to "StdUtils.t.sol" * Nit: remove unnecessary "bytes20" casts * style: add white-spaces in "deal" * fix: readd "deployCode" functions with "val" * Add "computeCreate2Address" utility Rename "testGenerateCorrectAddress" to "testGenerateCreateAddress" * refactor: use "console2" in "Utils.sol" * style: end lines and white spaces * test: drop pragma to ">=0.8.0" in "StdError.t.sol" chore: remove comment about "v0.8.10" in "Errors.sol" * refactor: define "vm" and "stdStorage" in "TestBase" feat: add "Components.sol" file which re-exports everything * fix: inherit from DSTest in Test * feat: ScriptBase refactor: delete "TestBase.sol" refactor: move TestBase in "Test.sol" * ♻️ Make assertions virtual * ♻️ Make deployCode virtual * ✨ (Components) Export consoles * ♻️ (Script) Import Vm * ♻️ Import from Components * ♻️ Make bound view Co-authored-by: Zero Ekkusu <94782988+ZeroEkkusu@users.noreply.github.com> * feat: make `Script` safer (#147) * feat: add `stdStorageSafe` * test(cheats): fix tests `deployCode` tests started failing after 01c60f9 * refactor: make components `abstract` * feat: add `CheatsSafe` * feat: add `VmSafe` * refactor: update `Script` * docs: add license info (#156) * feat: rebrand components (#157) * feat: rebrand components Rename to Std<Component> * fix: StdErrors -> StdError * chore: remove `.DS_Store` * fix: use `ABIEncoderV2` * test: correct test name * fix: add `CommonBase` * refactor: move test dir to root * Revert "refactor: move test dir to root" This reverts commit f21ef1a. * refactor: move test dir to root, update ci accordingly * style: configure and run forge fmt * ci: split into jobs and add fmt job * ci: update name and triggers * ci: remove name field * feat: better bound, ref #188 * fix: bound logs + remove unneeded line * fix: update require strings * refactor: clean up `Test` and `Script` - do not forge fmt Components import - do not import Safe Components in `Test` * fix: udpate bound to match forge's uint edge bias strategy * feat: add interfaces (#193) * chore: update function visibility * feat: add interfaces * fix: fix import * style: consistent spec style * chore: fix find/replace issue Co-authored-by: Zero Ekkusu <94782988+ZeroEkkusu@users.noreply.github.com> * chore: update comments Co-authored-by: Zero Ekkusu <94782988+ZeroEkkusu@users.noreply.github.com> Co-authored-by: Zero Ekkusu <94782988+ZeroEkkusu@users.noreply.github.com> * feat: reimplement `bound` w/ even distribution * build: rename step * Add memory-safe notation so that compiling via-ir can optimize effectively (#196) * test(bound): add even distribution test (#197) * feat: add `assumeNoPrecompiles` (#195) * refactor: use fully-qualified paths instead of relative paths * chore: fix typo * feat: start adding StdChains * feat: start adding assumeNoPrecompiles * feat: add chains * feat: add precompiles/predeploys * Revert "refactor: use fully-qualified paths instead of relative paths" This reverts commit bb2579e. * refactor: use relative paths for compatibility with solc <0.6.9 (no --base-path flag) * refactor: make assumeNoPrecompiles virtual * refactor: no more constructor warning from StdChains * fix: move stdChains into StdCheats, fix constructor initalization order, move cheats into VmSafe that can be safely used * ♻️ update ds-test (#200) * ♻️ update ds-test Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch> * ♻️ use relative path for ds-test imports Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch> Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch> * refactor: move `UINT256_MAX` to `CommonBase` Signed-off-by: Pascal Marco Caversaccio <pascal.caversaccio@hotmail.ch> Co-authored-by: Paul Razvan Berg <hello@paulrberg.com> Co-authored-by: Matt Solomon <matt@mattsolomon.dev> Co-authored-by: Drake Evans <31104161+DrakeEvans@users.noreply.github.com> Co-authored-by: Pascal Marco Caversaccio <pcaversaccio@users.noreply.github.com>
Closed by #184 |
Problem
bound
we had was zero: no problem—any x values < 1000 are unchanged (because 5 % 1000 = 5)bound(x, 75, 1000)
Potential solution
If
x
is within the range of the bound limits, do not shift the value. Basically add something likeif (x >= min && x <= max) return x;
. Mainly just need to make sure this doesn't introduce some weird bias/distribution, but I think it's ok and a good solutionThe text was updated successfully, but these errors were encountered: