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

Bytecode Verification #47

Merged
merged 38 commits into from
Apr 4, 2024
Merged

Bytecode Verification #47

merged 38 commits into from
Apr 4, 2024

Conversation

prateek105
Copy link
Collaborator

@ElliotFriedman
Copy link
Contributor

ElliotFriedman commented Mar 20, 2024

Merge conflicts will need to be resolved with the latest changes that were just merged.

It would be great to see documentation in FPS so that users can understand how to use this system. Documentation should include instructions for users on how to install FPS and install node modules so that this works.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ElliotFriedman
Copy link
Contributor

Unit tests are failing in CI with error:

Ran 1 test for test/TypeCheck.t.sol:TypeCheck
[FAIL. Reason: setup failed: failed to read from "/home/runner/work/addresses/Addresses.json": No such file or directory (os error 2)] setUp() (gas: 0)

@prateek105
Copy link
Collaborator Author

running

forge test --mc TypeCheck --ffi --fork-url sepolia

locally, this is the output:

No files changed, compilation skipped
2024-04-02T21:41:29.897864Z ERROR cheatcodes: non-empty stderr input=["npx", "ts-node", "typescript/encode.ts", "[[\"0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5\", \"0x95222290dd7278aa3ddd389cc1e1d165cc4bafe5000000000000000000000000\", [\"0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5\", 2, [\"0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5\", 2]]], [\"Arg1\", \"Arg2\"], [2, 3], [[\"0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5\", 2, [\"0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5\", 2]], [\"0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5\", 2, [\"0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5\", 2]]]]", "ExampleTypeCheck_02.sol:ExampleTypeCheck_02", "out/"] stderr="/Users/elliot/node_modules/@ethersproject/logger/src.ts/index.ts:269\n        const error: any = new Error(message);\n                           ^\nError: expected array value (argument=null, value=\"0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5\", code=INVALID_ARGUMENT, version=abi/5.7.0)\n    at Logger.makeError (/Users/elliot/node_modules/@ethersproject/logger/src.ts/index.ts:269:28)\n    at Logger.throwError (/Users/elliot/node_modules/@ethersproject/logger/src.ts/index.ts:281:20)\n    at Logger.throwArgumentError (/Users/elliot/node_modules/@ethersproject/logger/src.ts/index.ts:285:21)\n    at ArrayCoder.Coder._throwError (/Users/elliot/node_modules/@ethersproject/abi/src.ts/coders/abstract-coder.ts:68:16)\n    at ArrayCoder.encode (/Users/elliot/node_modules/@ethersproject/abi/src.ts/coders/array.ts:195:18)\n    at /Users/elliot/node_modules/@ethersproject/abi/src.ts/coders/array.ts:62:19\n    at Array.forEach (<anonymous>)\n    at pack (/Users/elliot/node_modules/@ethersproject/abi/src.ts/coders/array.ts:54:12)\n    at ArrayCoder.encode (/Users/elliot/node_modules/@ethersproject/abi/src.ts/coders/array.ts:210:16)\n    at /Users/elliot/node_modules/@ethersproject/abi/src.ts/coders/array.ts:62:19 {\n  reason: 'expected array value',\n  code: 'INVALID_ARGUMENT',\n  argument: null,\n  value: '0x95222290DD7278Aa3Ddd389Cc1E1d165CC4BAfe5'\n}\n"

Ran 3 tests for test/TypeCheck.t.sol:TypeCheck
[PASS] test_typeCheck() (gas: 6622503)
[PASS] test_typeCheckIncorrectArtifact() (gas: 2555888)
[PASS] test_typeCheckIncorrectByteCode() (gas: 5017304)
Suite result: ok. 3 passed; 0 failed; 0 skipped; finished in 9.57s (17.26s CPU time)

This is a required behaviour. This error corresponds to test_typeCheckIncorrectArtifact test. Where we are passing incorrect artifact path to TypeChecker, which leads to this typescript error while encoding. So we want this error.

Comment on lines +82 to +84
TYPE_CHECK_ADDRESSES_PATH=addresses/TypeCheckAddresses.json
ADDRESSES_PATH=addresses/Addresses.json
ARTIFACT_PATH=out/
Copy link
Contributor

Choose a reason for hiding this comment

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

missing lib path here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A user who has installed fps and is type checking his contracts doesn't need to think about lib path. We have this code vm.envOr("LIB_PATH", string("lib/forge-proposal-simulator/")); in script so lib path will be set by default. I have added this LIB_PATH env so that this script can also be run from the fps repo by setting LIB_PATH as "" just like we do in test.

```
TYPE_CHECK_ADDRESSES_PATH=addresses/TypeCheckAddresses.json
ADDRESSES_PATH=addresses/Addresses.json
ARTIFACT_PATH=out/
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing LIB_PATH

import {Strings} from "@openzeppelin/utils/Strings.sol";
import {Test} from "@forge-std/Test.sol";
import "@forge-std/console.sol";

Copy link
Contributor

Choose a reason for hiding this comment

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

add example of how to run from CLI from within FPS

TYPE_CHECK_ADDRESSES_PATH=addresses/TypeCheckAddresses.json ADDRESSES_PATH=addresses/Addresses.json ARTIFACT_PATH=out/ LIB_PATH= forge script script/TypeCheck.s.sol:TypeCheck --ffi --fork-url  sepolia

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done here ddea628

Copy link
Contributor

@ElliotFriedman ElliotFriedman left a comment

Choose a reason for hiding this comment

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

LGTM!

@ElliotFriedman ElliotFriedman merged commit 9bce630 into main Apr 4, 2024
3 of 5 checks passed
@ElliotFriedman ElliotFriedman deleted the feat/type-checking branch April 4, 2024 16:33
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