ERC725Alliance / erc725.js

Package to interact with ERC725 smart contracts
Apache License 2.0
38 stars 29 forks source link

Change the way erc725.js encode static `valueType` #277

Closed CJ42 closed 1 year ago

CJ42 commented 1 year ago

I'm submitting a...

Summary

Currently, the erc725.js library uses the ABI encoding method from etherjs. This leads to an encoding format for data that contain padding and offsets, as specified in the Contract ABI Specs.

The problem is here is that this encoding format is not valid ❌

Because erc725.js is a library used to encode data for storing in the storage of an ERC725Y smart contract, we should store plain arbitrary bytes, without any formatting or padding

Detailed informations

We will need to do quite a bit of refactoring in erc725.js for the encoding/decoding of types. At the moment, for every type, what is being used is the ABI encoding / decoding methods. This leads to ABI encode, meaning it adds padding + offsets depending on the type (see the screenshot).

Below is a summary of how each valueType should be encoded.

valueType Encoding Example
bool 0x01 (for true) or 0x00 (for false)
string as utf8 hex bytes
without padding ❌
"Hello" --> 0x48656c6c6f
address as a 20 bytes long address 0x388C818CA8B9251b393131C08a736A67ccB19297
uin256 as a hex value 32 bytes long
left padded with zeros to fill 32 bytes
number 5 --> 0x0000000000000000000000000000000000000000000000000000000000000005
uintN
(where N is a multiple of 8 in the range 8 > N > 256)
as a hex value N bytes long
left padded with zeros to fill N bytes
number 5 as uint32 --> 0x00000005
bytes32 as a hex value 32 bytes long
right padded to fill 32 bytes
0xca5ebeeff00dca11ab1efe6701df563bc1add009076ded6bcf4c6f771f2e3436
bytes4 as a hex value 4 bytes long
right padded to fill 4 bytes
0xcafecafe
bytesN (from 1 to 32) as a hex value N bytes long
right padded to fill N bytes
bytes as hex bytes of any length
without padding
0xcafecafecafecafecafecafecafecafe...

The only exception for ABI encoding so far is the case of array []. When one of the type above is appended with [] (e.g: bytes4[] or bool[]), it should be encoded according to the ABI format.

zcstarr commented 1 year ago

Not sure but wanted to add to this, not sure if this a part of the related changes , but for instance https://github.com/ERC725Alliance/erc725.js/blob/ab1605cd4e172630eed08353c185bb2c229cd80d/src/lib/encoder.ts#L416

It seems like the conversion from uint256 in the 725 storage breaks, when you store a uint256 sized int. I think the breaking thing would be parseInt() , which can at most only return a javascript max_uint number which I think would be a 64bits.

So I think there might be some work with valueContent as well to sort with encoding.

CJ42 commented 1 year ago

@zcstarr what would be your preference for what would be returned after decoding a very large uint256?

a BigNumber like in ethersjs?

https://docs.ethers.org/v5/api/utils/bignumber/#BigNumber--notes

zcstarr commented 1 year ago

I think my preference would be for something like that, then you could just get back the value. The current work around is to store things as bytes32, and then ultimately convert that response into BigNumber.

I think actually the preference might be for BigInt.

I think if consumers are using ethers, then they can slap that into BigNumber.from(BigInt) with current version. Ethers in next version is a migration to BigInt. Hard call to say if this is a useful transition now. Also looks like web3(unconfirmed 😁 ) is moving to bigInt or has as well https://github.com/search?q=repo%3Aweb3%2Fweb3.js%20bigint&type=code

https://docs.ethers.org/v6/migrating/#migrate-bigint

Hugoo commented 1 year ago

We does tag "invalid" mean? I'm not sure it is correct.

Hugoo commented 1 year ago

The lib does not use ethersjs atm? It uses: import AbiCoder from 'web3-eth-abi';

Not sure if i understand why you mention etherjs

CJ42 commented 1 year ago

We does tag "invalid" mean? I'm not sure it is correct.

Not sure neither. I thought it was to describe that something is invalid in the library. I can delete the tag.

The lib does not use ethersjs atm? It uses: import AbiCoder from 'web3-eth-abi';

Not sure if i understand why you mention etherjs

I was asking if it would be preferable to return numbers as BigNumber like in ethers.js, not normal JS numbers because of the max number limit in Javascript.

CJ42 commented 1 year ago

Closing as implemented in https://github.com/ERC725Alliance/erc725.js/pull/288