Make proxy to return bytes instead of bytes32

[gbalabasquer]

I actually have some doubts about the 5K gas we are leaving to finish the execution. Is that always enough? I've seen other implementations leaving 10K. We also have the PR from Robert allowing to set a variable for this. I'm not sure if that needs to be variable or if we can make sure with a certain amount we are always just fine.

[NiklasKunkel]

That’s actually exactly why I put in a 5k buffer. Gas prices of different operations were still in flux quite frequently and I wanted to make sure the contracts didn’t break immediately after such a change. That being said, a dynamic value would be much better.

On Wed, Oct 17, 2018 at 8:13 AM rain notifications@github.com wrote:

@rainbreak commented on this pull request.

In src/proxy.sol https://github.com/dapphub/ds-proxy/pull/18#discussion_r225973937:

 {

require(_target != 0x0);

     // call contract in current context
     assembly {

• let succeeded := delegatecall(sub(gas, 5000), _target, add(_data, 0x20), mload(_data), 0, 32)
• response := mload(0) // load delegatecall output
• let succeeded := delegatecall(sub(gas, 5000), _target, add(_data, 0x20), mload(_data), 0, 0)

@NiklasKunkel https://github.com/NiklasKunkel can you explain the original choice of 5000 here? I wonder if there is a way to make this more dynamic, in case of future CALL pricing changes,

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/dapphub/ds-proxy/pull/18#pullrequestreview-165686652, or mute the thread https://github.com/notifications/unsubscribe-auth/ABu7vSP6G8hySFi9dSMN0buVRNn5fx3Nks5ul0kPgaJpZM4XkGAS .

[Jake-Gillberg]

I actually have some doubts about the 5K gas we are leaving to finish the execution. Is that always enough? I've seen other implementations leaving 10K. We also have the PR from Robert allowing to set a variable for this. I'm not sure if that needs to be variable or if we can make sure with a certain amount we are always just fine.

The 5K gas isn't to finish the execution, is it? My understanding is that this is just the gas that is needed to get the delegatecall off the ground, that when we we return from delegatecall we have all the gas that the call didn't use. Happy to be told that I am wrong, but then I would also be confused as to why the previous tests worked with 703 gas reserved with G_Call being 700.. 😄

[NiklasKunkel]

This is correct, it’s exclusively the gas needed for delegatecall

On Wed, Oct 17, 2018 at 9:41 AM Jake Gillberg notifications@github.com wrote:

I actually have some doubts about the 5K gas we are leaving to finish the execution. Is that always enough? I've seen other implementations leaving 10K. We also have the PR from Robert allowing to set a variable for this. I'm not sure if that needs to be variable or if we can make sure with a certain amount we are always just fine.

The 5K gas isn't to finish the execution, is it? My understanding is that this is just the gas that is needed to get the delegatecall off the ground, that when we we return from delegatecall we have all the gas that the call didn't use.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/dapphub/ds-proxy/pull/18#issuecomment-430701556, or mute the thread https://github.com/notifications/unsubscribe-auth/ABu7vUfodNUC_Ymo9mHxc9dxHyZ5yTfoks5ul12_gaJpZM4XkGAS .

[Jake-Gillberg]

This is correct, it’s exclusively the gas needed for delegatecall

Where does the 3 come from in the 703? Is it the sub?

[NiklasKunkel]

Yes.

On Wed, Oct 17, 2018 at 9:52 AM Jake Gillberg notifications@github.com wrote:

This is correct, it’s exclusively the gas needed for delegatecall

Where does the 3 come from in the 703? Is it the sub?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/dapphub/ds-proxy/pull/18#issuecomment-430705503, or mute the thread https://github.com/notifications/unsubscribe-auth/ABu7vfFOkhc_jTtYUmUJHfKCktPp9sj-ks5ul2BkgaJpZM4XkGAS .

[gbalabasquer]

ahh ok got it.

[NiklasKunkel]

I reviewed the commits and this looks good to me now and ready for merging. @rainbreak do you disagree? Let's keep the discussion of the static (5000) vs dynamic gas cost for delegatecall separate from this PR.

[rainbreak]

@gbalabasquer @NiklasKunkel I think hevm/revert is working now, see https://github.com/dapphub/dapptools/pull/76 to try it out.

In the interactive mode there is now a memory view, along with return/call data, which is pretty useful for debugging this issue. Toggle it with m.

From what I can tell this PR is not working yet because of some Solidity behaviours:

1. Overriding the response value. If you do the revert from inside the assembly block the result doesn't contain the revert message bytes, but it does if you do the revert from outside e.g.

        string memory response;
        bool succeeded;
        assembly {
            succeeded := delegatecall(sub(gas, 5000), _target, add(_data, 0x20), mload(_data), 0, 0)
            let size := returndatasize
            response := mload(0x40)
            mstore(0x40, add(response, and(add(add(size, 0x20), 0x1f), not(0x1f))))
            mstore(response, size)
            returndatacopy(add(response, 0x20), 0, size)
        }
        // what is solidity doing to the return?
        if (succeeded) return bytes(response);
        else revert(response);

This has some cruft from the bytes/string conversion, but at least the returndata contains the correct bytes somewhere. It looks like Solidity is interfering somehow with the revert data, but I'm not sure how exactly.

2. Solidity message formatting. Solidity treats the messages in revert(msg) as string, not bytes.

From the docs https://solidity.readthedocs.io/en/v0.5.0/control-structures.html#error-handling-assert-require-revert-and-exceptions (at the bottom)

The provided string will be abi-encoded as if it were a call to a function Error(string). In the above example, revert("Not enough Ether provided."), will cause the following hexadecimal data be set as error return data:

0x08c379a0                                                         // Function selector for Error(string)
0x0000000000000000000000000000000000000000000000000000000000000020 // Data offset
0x000000000000000000000000000000000000000000000000000000000000001a // String length
0x4e6f7420656e6f7567682045746865722070726f76696465642e000000000000 // String data

This explains some of the structure of the returndata, the rest possibly being bytes/string casting (but I'm not sure at all).

It appears that Solidity (all 0.4.24 for me) is too opinionated here. A possible solution is to do everything in assembly to avoid these issues (which would mean not doing the solidity internal call from execute(code, data)).

[rainbreak]

p.s. Also, I changed ds-test a bit locally for more useful debugging:

diff --git a/src/test.sol b/src/test.sol
index 2de2d90..d64d0fc 100644
--- a/src/test.sol
+++ b/src/test.sol
@@ -16,6 +16,7 @@ pragma solidity ^0.4.23;
 contract DSTest {
     event eventListener          (address target, bool exact);
     event logs                   (bytes);
+    event logs_named             (bytes32 key, bytes val);
     event log_bytes32            (bytes32);
     event log_named_address      (bytes32 key, address val);
     event log_named_bytes32      (bytes32 key, bytes32 val);
@@ -131,8 +132,8 @@ contract DSTest {

         if (!ok) {
             emit log_bytes32("Error: Wrong `bytes' value");
-            emit log_named_bytes32("  Expected", "[cannot show `bytes' value]");
-            emit log_named_bytes32("  Actual", "[cannot show `bytes' value]");
+            emit logs_named("  Expected", b);
+            emit logs_named("    Actual", a);
             fail();
         }
     }

[gbalabasquer]

So I've made the change for the revert case to forward the message correctly. IMO it is working as it should be, for both cases, the solidity require/revert and also for a pure assembly revert. If you use the assembly one, you will not be receiving the whole error structure like solidity one does. The proxy just returns the revert message as it came from the called contract.

[gbalabasquer]

@rainbreak do you think we could already merge this?