-
Notifications
You must be signed in to change notification settings - Fork 0
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
Code Jar Improvements with Stub #137
Conversation
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.
hmmm, some of the gas overhead might be worth looking into.
are we only seeing overhead for tests that use ScriptSource
and so do a deploy? 🤔
quark-core/src/CodeJarStub.yul
Outdated
object "CodeJarStub_9" { | ||
code { | ||
/// @src 0:66:211 "contract CodeJarStub {..." | ||
mstore(64, 128) |
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 pretty lol, but this is all that ends up executing, I believe. since constructor_CodeJarStub_9
is our code that returns. I believe the optimizer is smart enough to optimize the rest out when generating bytecode.
Looks like the overhead is only affecting tests using Agreed, would be good to confirm where the extra overhead comes from. |
quark-core/src/CodeJar.sol
Outdated
assembly { | ||
codeCreateAddress := create2(0, add(initCode, 32), initCodeLen, 0) | ||
} | ||
CodeJarStub codeCreateAddress = new CodeJarStub{salt: 0}(code); |
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.
nit: not technically an address, think a name like createdCode
is more fitting
quark-core/src/CodeJar.sol
Outdated
* @dev Returns the create2 address based on the given initCode | ||
* @return The create2 address based on running the initCode constructor |
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.
Outdated comments
86eeaba
to
f89eec4
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.
LGTM. Reminder to remove the yul code and re-generate the gas diffs.
In the meantime, I'll try to investigate why there is a sizable gas overhead for some of the tests.
Looks like the gas overhead is coming from the initcode for Though, I guess there's nothing we can do other than try to play around with compiler settings to optimize |
f89eec4
to
0499de8
Compare
This patch improves Code Jar to use CodeJarStub, which is a stub that's semi-verifiable on Etherscan for deployments, and significantly reduces the complexity/noise of how CodeJar works. This is also the basis for CodeJar deployments, which will be included in a different branch.
0499de8
to
ca843bb
Compare
This patch just hacks in `datasize("CodeJarStub")` as a magic constant (derived emperically) into the contract itself, which allows us to get an optimal version of CodeJarStub that saves ~2-3K gas from using Solidity's ABI-encoding which is particularly expensive mostly due to dynamic allocations that break the optimizer. This is a weird system, but read the comments as to why it's probably an okay decision.
This patch improves Code Jar to use CodeJarStub, which is a stub that's semi-verifiable on Etherscan for deployments, and significantly reduces the complexity/noise of how CodeJar works. This is also the basis for CodeJar deployments, which will be included in a different branch.