Util: should stripZeros really strip the last 0 as well?

[holgerd77]

The stripZeros() function is intended to trim leading zeros from a Buffer or an Array, e.g.:

"0x034f" -> "34f"
"0x03" -> "3"

In the current implementation a zero value 0x00 is also trimmed down to an empty string like:

"0x00" -> "" or also
"0x0" -> ""

Should this really be the case respectively is this intended (e.g. in the context of a simplified Ethereum zero value representation or something, stumbled e.g. on this (see Point 3.)?

This issue should be handled with care, this would also change the behavior of the defineProperties() creator on the allowLess setting and e.g. trickle down to a changed ethereumjs-tx Transaction initialization on zero values.

[holgerd77]

@davidmurdoch You could already help very much if you give an explicit stance on this here. Read closely what I wrote up above and go along where it might make sense to clarify, confirm or further investigate. Thanks!

[davidmurdoch]

@holgerd77, I'd expect a function named stripZeros to strip all zeros.

I think the confusion on if a 0 quantity should be represented by Buffer.from([0]) (0x0) or Buffer.alloc(0) (0x) probably stems from the fact that we have to deal in both the Ethereum JSON RPC spec as well as Ethereum's state machine. The JSON RPC spec defines hex encoding for two data types: UNFORMATTED DATA and QUANTITY, whereas the ethereum whitepaper makes no distinction of its own (from what I can tell, maybe an RLP spec somewhere defines this behavior in detail) and the go-ethereum source code explicitly treats uint 0 as an empty RLP string.. So, in the context of JSON-RPC, a transaction with "nonce": 0, for example, should be represented as 0x0, but in the context of signing this same transaction, it should be treated as 0x -- otherwise transaction hashes will differ.

What may need to happen is that ethereumjs-tx/util's toJSON method may need to know the appropriate JSON-RPC return types (DATA vs QUANTITY) for the underlying fields, if JSON-RPC formatting is the intent of the method. As it is now, tx.nonce = "0x0"; assert.equal(tx.toJSON().nonce, "0x0"); fails.

To complicate matters further, I could see FakeTransaction needing to differentiate between nonce == "0x"/null/undefined/Buffer.from([]) and nonce == 0/"0x0"/Buffer.from([0]), but this is because I'm looking to refactor the way ganache-core utilizes Transaction/FakeTransaction and it'd be useful to differentiate there.

[holgerd77]

@davidmurdoch Whew, had a first deep-read on this (also corrected some typos), and I think I got the problem, thanks for this!

[holgerd77]

I have the feeling that we can do much much more on things like this once we go full TypeScript (cc @krzkaczor) don't have the full/clear picture here yet though.

[danjm]

@davidmurdoch I sketched up some quick code to check if I understand your explanation correctly.

it is a simple and naive implementation of the idea that

What may need to happen is that ethereumjs-tx/util's toJSON method may need to know the appropriate JSON-RPC return types (DATA vs QUANTITY) for the underlying fields, if JSON-RPC formatting is the intent of the method. As it is now, tx.nonce = "0x0"; assert.equal(tx.toJSON().nonce, "0x0"); fails.

The changes for ethereumjs-tx can be found here ethereumjs/ethereumjs-tx@prepend-0x-to-tx-data...quantity-fields-allow-zero-proof-of-concept
And for ethereumjs-util: https://github.com/ethereumjs/ethereumjs-util/compare/quantity-fields-allow-zero-proof-of-concept

I'll note that it deviates from your suggestion in that it is the setters and the signing function, and not the toJSON function, which "know the appropriate return types", but the effect should be the same.

When you get a chance, can you check these? Does it accurately reflect a simple application of the general strategy you outlined?

[davidmurdoch]

@danjm Looks good as far as preserving the original data. One thing to note here is that if the toJSON method is now intended to output JSON-RPC hex-encoded data 0x00 is invalid for a transaction's nonce field. The JSON RPC hex representation of 0 for a QUANTITY field is "0x0", not "0x00" (similarly, 1 is "0x1" and so on). But this would be another issue entirely (ganache doesn't use ethereumjs-tx's toJSON so we haven't had to worry about it).

Additionally, we may need to ensure that when signing the raw transaction data it does not include the 0 -- I'm not 100% positive this is correct -- you'll just want to check that transactions still result in the same signature as before.

[s1na]

I'm not sure hiding more complexity in defineFields is a sustainable approach! I'd be generally more in favor of dropping the use of util methods in different contexts with different requirements and moving data manipulation closer to the logic.

[danjm]

@s1na Yes, that is a very good suggestion. I am going to put together a draft PR that attempts to do this while accounting for the issue raised by @davidmurdoch

I will have time to work on that on the weekend, will get a PR up by Monday (Apr 4)

[holgerd77]

Hmm, I am still not sure if there might be something to be drawn from this issue, e.g. an additional boolean switch for the stripZeros() function.

Will for now keep the issue and transfer to the monorepo.

[holgerd77]

Related: ethereumjs/ethereumjs-util#79

[holgerd77]

Outdated, will close.