Fix extraData in approveAndCall not being ABI encoded in HumanStandardToken

[izqui]

Appending a bytes array to a function call wasn't properly setting it as the bytes parameter.

[simondlr]

Hey @izqui. I misunderstood how you changed/edited it from our Twitter convo.

I don't think this is correct. You should be able to pass bytes as usual into extraData (from Solidity itself), in which this will cause it to break. (eg: using a throwProxy here by passing through bytes will cause this to break).

Meaning, the responsibility for the right encoding should not be the contract's responsibility. It's the test itself that should change its encoding, and then pass it onto the contract.

Does that make sense?

As far as I understand it, you are basically allowing the contract to take non-bytes info and create the equivalent bytes representation? Is that correct?

[izqui]

The problem is that, as far as i understand, passing a bytes array directly in a raw call will not correctly ABI encode it as a bytes array parameter, it will just append those bytes. In order for making a correct call, this encoding needs to be made.

…

On Sat, Apr 1, 2017, 4:37 PM Simon de la Rouviere ***@***.***> wrote: Hey @izqui <https://github.com/izqui>. I misunderstood how you changed/edited it from our Twitter convo. I don't think this is correct. You should be able to pass bytes as usual into extraData (from Solidity itself), in which this will cause it to break. (eg: using a throwProxy here by passing through bytes will cause this to break). Meaning, the responsibility for the right encoding should not be the contract's responsibility. It's the test itself that should change its encoding, and then pass it onto the contract. Does that make sense? As far as I understand it, you are basically allowing the contract to take non-bytes info and create the equivalent bytes representation? Is that correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbTYEUT0sOqLtws-T4bUb4JqzLcvWPmks5rrmEWgaJpZM4Mvnvy> .

[simondlr]

@izqui, yes. That is correct, but I would still venture and say it should be encoded before you do the call. The contract shouldn't do the encoding.

[izqui]

Ok, I see what you are saying.

But I think the user expectation is that whatever data they send in the extraData parameter is exactly what the receiving contract will receive in the extraData parameter.

What you are proposing right now is that for calling an approveAndCall, the user would need to do:

token.approveAndCall(..., abi_encoded(extraData))

And that extraData parameter will be ABI encoded by web3 again, so the data sent to the blockchain in the tx data will be abi_encoded(abi_encoded(extraData)).

I think it should be a responsibility of the contract once it receives a valid bytes array to pack it correctly doing its abi_encode before forwarding it via a raw call.

[simondlr]

I think it should be a responsibility of the contract once it receives a valid bytes array to pack it correctly doing its abi_encode before forwarding it via a raw call.

I might be misunderstanding this.

The issue I have with this is that an already encoded call will be then malformed by the right pad. This is likely to occur when you use bytes that are already correctly encoded: eg msg.data. So, then you would need to decode this, JUST to be able to send it across.

You are also then incurring extra gas costs, when you could've just encoded it outside the contract.

Do I have it right?

[izqui]

The problem is that when you do a raw call, and pass a bytes array as part of the payload it will not encode it correctly.

The extra data is correctly encoded in msg.data but gets decoded when assigning to the extraData parameter.

I'm doing a poor job explaining myself, let me try with this example:

contract Receiver {
  function receive(bytes extraData);
}

contract Sender {
  function send(address to, bytes extraData) {
      Receiver(to).receive(extraData);  // this works

      bool result1 = to.call(bytes4(sha3('receive(bytes)')), uint256(byte(0x80)), uint256(extraData.length), rightPad(extraData));  // works as the above function

      bool result2 = to.call(bytes4(sha3('receive(bytes)')), extraData);  // this will not work as the above call as it just appends the decoded bytes to the call payload.
  } 
}

[skmgoldin]

it should be encoded before you do the call. The contract shouldn't do the encoding.

I agree with Simon. No reason to spend gas on this in-EVM. Better separation of concerns leaving it out of the contract. I think this should be closed.

[simondlr]

There is an issue iirc, that I spoke with @izqui about. And I tried to get a grip around the problem by running some tests and encodings. Encoding issues in general is a headache for me though. And I forgot exactly what the problems/issues were.

[izqui]

I think the easiest way to solve this is by having Solidity do the encoding under the hood, and not doing a raw call.

MiniMe fixed this having an interface and then just making the call through it

[GNSPS]

I've reviewed this and @izqui claim is right. However there's not a need to right pad the extra data.

My proposal is to keep the call as:

_spender.call(bytes4(bytes32(sha3("receiveApproval(address,uint256,address,bytes)"))), msg.sender, _value, this, uint256(byte(0x80)), uint256(_extraData.length), _extraData)

And drop the rightPad() function altogether.

Other than that this LGTM, we should merge! 😄

[GNSPS]

Could you please remove both the rightPad() function and its use in one of the raw call's parameters?

[maurelian]

approveAndCall is not part of the EIP20 token definition, and for the time being, not within scope of this repo.