-
Notifications
You must be signed in to change notification settings - Fork 196
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
refactor(store): update tightcoder codegen, optimize TightCoder library #1210
Conversation
🦋 Changeset detectedLatest commit: f09aed7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
84ef75c
to
806419b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good, just minor nits
packages/store/gas-report.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice savings 🔥
packages/store/package.json
Outdated
@@ -46,7 +46,7 @@ | |||
"clean:typechain": "rimraf types", | |||
"dev": "tsup --watch", | |||
"gas-report": "mud-gas-report --save gas-report.json", | |||
"generate-tightcoder": "tsx ./scripts/generate-tightcoder.ts && prettier --write '**/tightcoder/*.sol'", | |||
"generate-tightcoder": "tsx ./ts/scripts/generate-tightcoder.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we call this build:tightcoder
for consistency with the other build scripts? (no strong opinion)
"generate-tightcoder": "tsx ./ts/scripts/generate-tightcoder.ts", | |
"build:tightcoder": "tsx ./ts/scripts/generate-tightcoder.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's based on generate-test-tables
from cli
, I also don't have a strong opinion either way.
generate-test-tables
and generate-tightcoder
are different so it's okay for them to diverge too (test tables are for tests only, tightcoder is for everything)
@holic thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they don't get in the way (e.g. creating a circular dependency), I'd be fine moving them both to build:*
so we can get them in the "build everything" pipeline and not miss changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed build:tightcoder in this PR, test-tables in #1248
function testEncodeBytesArray() public { | ||
bytes[] memory input = new bytes[](2); | ||
input[0] = new bytes(32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did this test go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't support bytes[]
, string[]
, the test was for a function that's not used anywhere (which I also removed)
Co-authored-by: alvarius <[email protected]>
…ry (#1210) Co-authored-by: alvarius <[email protected]>
Extracted from #1183
Fixes parts of #1132
lumping codegen update and optimizations together cause
TightCoder.sol
gets different arguments and codegen is for the new methods