ethereumjs-abi icon indicating copy to clipboard operation
ethereumjs-abi copied to clipboard

Solidity pack Array support

Open vrolland opened this issue 7 years ago • 13 comments

Solidity pack Array support

vrolland avatar Sep 13 '17 14:09 vrolland

Hum, I am quite confused by the automatics tests. They fail during the npm install, right?

vrolland avatar Sep 13 '17 15:09 vrolland

Any feedback, please?

vrolland avatar Sep 14 '17 08:09 vrolland

Hmm, this library hasn't been touched in a while and I'm not sure if anyone feels actively responsible right now.

I'm not really that much into Solidity/API stuff and not feeling competent enough in this area to make a review. Maybe ask @axic, since he is doing both ethereumjs-vm and Solidity work, but it will probably take a few days, please be a bit patient.

holgerd77 avatar Sep 14 '17 08:09 holgerd77

No problem, I didn't want to be that pushing. I am very impressed by your job guys, I just want to help. Cheers!

vrolland avatar Sep 14 '17 11:09 vrolland

@vrolland thanks for the PR! Can you please explain the feature?

axic avatar Dec 06 '17 03:12 axic

Hello axic !

commit : c5d4746befd149e8d4169ac6b628e3eefab78547 - add toSolidityBytes32 => add the function toSolidityBytes32() which mime the casting from many types (bytes*, bool, address, int*, uint*) to bytes32. Usefull to pass generic parameters to a solidity function in bytes32 and then parse it back to a specif type in the contract.

commit 5acbaf1567b046182443a73ed086a89160cc9a6a - Solidity pack Array support => add to the existing function solidityPack(), the support of arrays (bytes*[], bool[], address[], int*[], uint*[]). Usefull to mime array packing (e.g: to mime a solidity function like keccak256() with arrays)

vrolland avatar Dec 06 '17 11:12 vrolland

Oh I see, thanks, it makes sense. I'd prefer to have the two changes separate and lets tackle the support for array first here. I think instead of code duplication you could recursively call solidityPack inside the array case.

Would you be interested updating the PR accordingly?

axic avatar Dec 06 '17 11:12 axic

I'd prefer to have the two changes separate and lets tackle the support for array first here.

Sure, I can make two different PR.

I think instead of code duplication you could recursively call solidityPack inside the array case.

Actually, this is not possible for most of the types. The array packing behavior is different than the regular packing. All the variables will be put in a format bytes32 (in arrays the data are not really packed). I may be able to use "toSolidityBytes32" in solidityPack for arrays, but in this case, I would make only one PR.

What do you think is the best?

vrolland avatar Dec 06 '17 12:12 vrolland

Why would it be different? This is the documentation we have for this format: http://solidity.readthedocs.io/en/develop/abi-spec.html#non-standard-packed-mode

Do you have some test cases which triggers this in Solidity?

axic avatar Dec 06 '17 12:12 axic

Yes, I actually already use this code in a project.

Solidity code : https://github.com/RequestNetwork/Request_SmartContracts/blob/master/contracts/synchrone/RequestEthereum.sol line 540 (see function getRequestHash() with a bytes[9] in parameters)

JS code : https://github.com/RequestNetwork/Request_SmartContracts/blob/master/test/synchrone/ethereum/requestEthereumBroadcastSignedRequestAsPayer.js line 49 (the function hashRequest() mime the solidity getRequestHash()) As you can see in the same file (line 10) I use a modified version of "ethereumjs-abi" with the commits I did here.

But, I may did somehting wrong.

vrolland avatar Dec 06 '17 12:12 vrolland

Hi, Anything I can do to make it happens?

vrolland avatar Jan 15 '18 02:01 vrolland

Guys, I added a commit to allow array with a dynamic size

vrolland avatar Feb 08 '18 08:02 vrolland

@axic Also can't judge on the validity of this PR here, needs some further discussion/clarification.

holgerd77 avatar Sep 21 '18 12:09 holgerd77