[StorageAccessible] invokeStaticDelegatecall #38
Conversation
rmeissner
left a comment
rmeissner
left a comment
There was a problem hiding this comment.
Should we separate this into multiple contracts that extend each other? This way it is oossible that you say I don't care about the static call logic, I rather safe some bytes.
|
Whats the status of this PR? Can it be merged as is? |
fleupold
left a comment
fleupold
left a comment
There was a problem hiding this comment.
Sorry, I had forgotten about this pending PR.
Should we separate this into multiple contracts that extend each other?
Do we already have a use case for a smart contract that would only like to use part of the functionality? If so, I'm happy to break it up into an inheritance hierarchy to allow feature selection. Otherwise, I'd slightly prefer keeping it in one contract as it makes it easier to read/understand the code in one go.
bh2smith
left a comment
bh2smith
left a comment
There was a problem hiding this comment.
Seems like this is ready to rock... although there is still an unanswered question in the discussion thread!
|
Regarding havin a separate contract: for me personally I would not require this method in the safe contracts and if possible would leave it out if I hit the contract maximum size. But as we have enought buffer right now it should not be critical. Why doesn't the static call for me hold a lot of value? For me the important part is that the contract calling uses a static call. It is actually not important that the inner call is a staticcall. So this could also be solved by providing an alternative interface that can be used by other contracts here the A static call is a protection mechanism and the question for me is, who do we try to protect here? |
|
Sorry, I thought this was ok to be merged (I incorrectly didn't see the comment as actually suggesting a change).
I don't consider this as much a protection mechanism, but more a convenience feature if another smart contract wants to call an extension from within its view function. E.g. if a GPv2 Oracle contract was exposing a view function that internally wanted to simulate a call on a GPv2 Reader Extension it would use @rmeissner pointed out that this is possible by just exposing an interface that marks the same function as view (I was under the impression the Given we have the use case as a unit test now, I will try to implement this suggestion in a follow up PR. Sorry again for asking to merge early. |
As per @rmeissner's suggestion in #38. This is a much more elegant way of achieving the same thing (no code duplication). Unfortunately this is an API breaking change and we already published the npm package vor v3.0.0. Maybe we can deprecate this package and republish this without a major version update? ### Test Plan unit test still passing.
Previously when other smartcontracts wanted to invoke
simulateDelegatecallthe callsite could not be a view method (sincesimulateDelegatecallitself isn't a view method).While there are good use cases for allowing non static invocations of
simulateDelegatecall(e.g. when we want to simulate some statechange and then query data), it can be inconvenient (e.g. in js we always have to .call since otherwise this would send a tx.This PR adds another method on the StorageAccessible contract that wraps the inner call in a
staticCallthus allowing the callsite to be declared as view.We need to discuss if the added bytecode is worthwhile this feature or if there are better options.
Test Plan
Added unittests