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.

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

feat: make Script safer #147

Merged
merged 6 commits into from
Aug 14, 2022
Merged

feat: make Script safer #147

merged 6 commits into from
Aug 14, 2022

Conversation

ZeroEkkusu
Copy link
Collaborator

@ZeroEkkusu ZeroEkkusu commented Aug 7, 2022

Motivation

Fix #78

Note: Merging into v0.3

Solution

Changes

  • Add stdStorageSafe without checked_write
  • Add CheatsSafe with only deployCode
  • Add VmSafe with only read-only cheatcodes
  • Update Script
  • Make all components abstract
  • Update deployCode tests

Notes

  • stdStorage delegates some functions to stdStorageSafe. This solution works for Solidity 0.6.0 and requires minimal code duplication:
stdStorageSafe {
    safeFun() {
        implementation
    }
}

stdStorage {
    safeFun() {
        return stdStorageSafe.safeFun()
    }

    unsafeFun() {
        implementation
    }
}
  • Interfaces cannot inherit in Solidity 0.6.0, so, VmSafe contains duplicate code. Someone should check if I removed any necessary cheatcodes in VmSafe.

Breaking changes

  • Only Cheats from CheatsSafe will be accessible in Script by default
  • Only cheatcodes from VmSafe will be accessible from vm in Script by default

Link #139

@ZeroEkkusu ZeroEkkusu requested a review from mds1 August 7, 2022 19:45
@ZeroEkkusu ZeroEkkusu marked this pull request as ready for review August 7, 2022 19:48
@ZeroEkkusu ZeroEkkusu mentioned this pull request Aug 11, 2022
7 tasks
// Derive a private key from a provided mnenomic string (or mnenomic file path) at the derivation path {path}{index}
function deriveKey(string calldata, string calldata, uint32) external returns (uint256);
}

interface Vm {
Copy link
Member

Choose a reason for hiding this comment

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

Should this inherit from VmSafe? Or it cannot because of older solidity?

Copy link
Collaborator

@mds1 mds1 Aug 12, 2022

Choose a reason for hiding this comment

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

Looks like interface inheritance was introduced in 0.6.2 https://github.com/ethereum/solidity/blob/develop/Changelog.md#062-2020-01-27

A bit hacky, but if we need to support versions older than 0.6.2 and don't want to duplicate code, we could probably make Vm and VmSafe abstract contracts instead of interfaces

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the pragma on master is pragma solidity >=0.6.0 <0.9.0;. Based on the changelog, I'd be surprised if anyone needed 0.6.0 or 0.6.1 and can't bump to 0.6.2, so I think bumping that pragma to enable interface inheritance is safe.

I know some Maker repos are on 0.6.x, but from a quick search it looks like they use ^0.6.12 so no issues there:

@mds1
Copy link
Collaborator

mds1 commented Aug 12, 2022

PR looks good to me overall 👌

Only Cheats from CheatsSafe will be accessible in Script by default

Only cheatcodes from VmSafe will be accessible from vm in Script by default

Where do you inherit from if you want all cheats in a script?

@ZeroEkkusu
Copy link
Collaborator Author

Where do you inherit from if you want all cheats in a script?

For Cheats:

contract MyScript is Script, Cheats {

For cheatcodes:

contract MyScript is Script {
      Vm internal constant VM = Vm(VM_ADDRESS);

You cannot name it vm, so this is up for discussion.

@mds1
Copy link
Collaborator

mds1 commented Aug 13, 2022

Got it, seems reasonable. Also should we rename to StdCheats[Safe] and StdAssertions? That naming fits with stdStorage, stdMath, etc., and makes it easier to different StdCheats (forge-std helpers) from cheatcodes (native forge cheatcodes), whereas IMO "Cheats" vs "cheatcodes" is an easy-to-confuse distinction. The files could all be renamed accordingly too for clarity (open to doing this in a separate PR, just thought of it since the above was a bit confusing)

@ZeroEkkusu
Copy link
Collaborator Author

I'll add those ideas to our todo.

@ZeroEkkusu ZeroEkkusu merged commit aa2e9a3 into v0.3 Aug 14, 2022
@ZeroEkkusu ZeroEkkusu deleted the safer_script branch August 14, 2022 19:56
@ZeroEkkusu ZeroEkkusu mentioned this pull request Sep 21, 2022
2 tasks
mds1 added a commit that referenced this pull request Oct 31, 2022
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants