Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add memory-safe notation so that compiling via-ir can optimize effect… #196

Merged
merged 1 commit into from
Oct 26, 2022
Merged

Add memory-safe notation so that compiling via-ir can optimize effect… #196

merged 1 commit into from
Oct 26, 2022

Conversation

DrakeEvans
Copy link
Contributor

…ively

@mds1
Copy link
Collaborator

mds1 commented Oct 25, 2022

Why do we need this? The docs say this comment style is going to be disallowed in a future release, so I'm hesitant to add this since (1) it'll break in the future, and (2) we don't really need to optimize console logs

@DrakeEvans
Copy link
Contributor Author

DrakeEvans commented Oct 26, 2022 •
edited
Loading

It is necessary because the IR compiler (via-ir flag) requires assembly to be annotated as memory safe in order to properly optimize code. See more information here: https://blog.soliditylang.org/2022/03/16/solidity-0.8.13-release-announcement/ relevant information at the heading: "Memory-Safe Assembly / Stack Too Deep"

If you look at the other places assembly blocks are used throughout the code base (including console.sol), they are annotated except for this one.

If we want to change the annotation style to assembly ("memory-safe") {...} I would be happy to do that throughout the code base

@mds1
Copy link
Collaborator

mds1 commented Oct 26, 2022

Yea I'm familiar with what it is, just wasn't sure why we were concerned about optimizing the test contracts. But I didn't realize we were already using this elsewhere, so given that I'm good with adding this and we can revisit once that breaking change is introduced 👌

However, do you mind changing this PR to merge into #184 instead (the v0.3 branch)? We're planning to merge that soon so new things are going directly into that branch for an upcoming forge-std release.

(forge-std supports as old as 0.6.2 so we can't use assembly ("memory-safe") since that will break compatibility with the older versions)

@DrakeEvans DrakeEvans changed the base branch from master to v0.3 October 26, 2022 19:13
@DrakeEvans
Copy link
Contributor Author

@mds1 Ive updated the base to be v0.3

mds1
mds1 approved these changes Oct 26, 2022
View reviewed changes
Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mds1 mds1 merged commit 6ca63a3 into foundry-rs:v0.3 Oct 26, 2022
mds1 added a commit that referenced this pull request Oct 31, 2022
@ZeroEkkusu @PaulRBerg
@mds1 @DrakeEvans @pcaversaccio
feat: forge-std v1.0.0 (#184)
17656a2 
* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* chore: update comments

Co-authored-by: Zero Ekkusu <[email protected]>

Co-authored-by: Zero Ekkusu <[email protected]>

* 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 <[email protected]>

* ♻️  use relative path for ds-test imports

Signed-off-by: Pascal Marco Caversaccio <[email protected]>

Signed-off-by: Pascal Marco Caversaccio <[email protected]>

* refactor: move `UINT256_MAX` to `CommonBase`

Signed-off-by: Pascal Marco Caversaccio <[email protected]>
Co-authored-by: Paul Razvan Berg <[email protected]>
Co-authored-by: Matt Solomon <[email protected]>
Co-authored-by: Drake Evans <[email protected]>
Co-authored-by: Pascal Marco Caversaccio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mds1 mds1 mds1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DrakeEvans @mds1