-
Notifications
You must be signed in to change notification settings - Fork 38
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
Rewrite execTransactions #59
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now, there are a bunch of issues which while they may seem unrelated are related in a lot of ways. There's a bunch of work necessary to get this thing to always "be able to do batch transactions while simultaneously constructing a contract proxy that will stick with some account forever, and also make errors happen in an informative manner if possible."
It has to work in real scenarios (#55). For the most part, this seems to be true, as even though the test suites are failing, they aren't failing that badly. (#58)
One reason for the failing tests has to do with it currently being impossible to send
safeTxGas
to theCPKFactory
for the first batch of transactions (#45). Doing that would help make estimating gas costs for these transactions more consistent with the already-existing-proxy use case as well, since the current gas estimation work does not do anything for proxies which don't exist yet. This has to do with thesafeTxGas
causing reverts if there's not enough gas available toexecute
the transaction, so the JSONRPCeth_estimateGas
would actually generally sort of work, and actually passing agas
value to the transaction options would be unnecessary.Actually getting a value for
safeTxGas
is tricky, as the value is returned asrevert
data from a call torequiredTxGas
. Different clients handle that differently, and there would probably have to be some way of doing this for the non-existent-proxy use case. This is kinda tricky becauserequiredTxGas
has to be issuedfrom
the proxy as well.Solving that would require changing the CPKFactory code, which would then involve redeploying the factory, and that would probably involve all contract changes planned for the next release. Of course, we probably shouldn't just leave all v1.1.1 behind with new improvements, so the strategy for handling new proxy addresses should be hashed out (#38), though perhaps we could put this off until later.
Contract changes related to being able to send
safeTxGas
to theCPKFactory
include:CPKFactory
metatransaction compatible in a way similar to how the Safe is. (GAS relayer support #19)payable
. (Make deployment call on factory payable #39) Sort of depends on Closes #186: Make execTransaction payable safe-global/safe-smart-account#202 being released, whereexecTransaction
is being updated in a similar way. This would allow themsg.sender
to send an ETHvalue
along with the overall transaction. Also related to the first point, in that maybe the user wants to use some ERC20 asgasToken
with a relayer for their transactions.These changes imply a major overhaul of the
CPKFactory
.Anyway, I can't do everything above at once. My plan is that this PR does the following: