-
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
Add event for ExecuteQuarkScript #130
Conversation
The event actually adds a fair bit of gas overhead (~3k+). The total cost depends on the length of the |
Cut the gas overhead down from >2.5k gas to ~1.8k gas by removing the It would be nice to keep the calldata as an event parameter, but it just adds too much overhead due to being arbitrary bytes. I considered only emitting the 4-byte function selector from the calldata, but we would need to specifically handle calldata with less than 4 byte lengths, which is possible for scripts with fallbacks. |
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.
overall looks good to me, will 👍 on resolution of one comment.
the factory test gas diffs seem large; is the branch up to date with |
d675008
to
ba4d78f
Compare
Just rebased. Will wait to see the refreshed gas diffs. |
Gas diffs look much smaller now, around 5k extra gas per deploy of the wallet. |
ba4d78f
to
55569c4
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.
I as as long as the gas is not increased to much I always in favor of having more logs hehe.
Quark wallet now emits an event when executing a Quark script. We purposely don't emit
scriptCalldata
as part of the event since that can add a lot of gas. The current gas overhead is ~1.7k.