Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v5 hex data is odd-length #614

Closed
wighawag opened this issue Sep 27, 2019 · 4 comments
Closed

v5 hex data is odd-length #614

wighawag opened this issue Sep 27, 2019 · 4 comments
Labels
enhancement New feature or improvement.

Comments

@wighawag
Copy link

Hey, when switching from v4 to v5 and also coming from web3 to v5,I keep encounterring issues with "hex data is odd-length"

I guess ethers.js decided to be more strict to avoid issues? Would like to know more the reasoning behind it

But could the entry points like sentTransaction and contract methods be more lenient ? Like it would pad the data automatically ?

@ricmoo
Copy link
Member

ricmoo commented Sep 27, 2019

Yes, v5 has it built into the hexlify and arrayify functions, with an option to allow odd-length, but generally this is not enabled internal to the library, except in very specific cases.

It is not safe to automatically pad; I've had several polls giving a developer the question of things like what do you expect 0x123 to be padded to, and it is about 60/40 split between 0x0123 and 0x1230, which is fair, since the first is how padding would work if the data were a numeric and the latter is how padding would work for a bytes.

Anything that enforces the even data length in ethers are places where there should never be odd-length hex strings anyways. How are you getting an odd-length transaction or data? This seems to me there is likely something incredible dangerous is happening, for example, manually computing the data and not padding it, which will almost certainly lead to invalid data...

I'm super curious what web3 code leads to this error, because that error should basically never show up... :s

@ricmoo ricmoo added the discussion Questions, feedback and general information. label Sep 27, 2019
@wighawag
Copy link
Author

BN.js output 123 for new BN(291).toString(16)
And I was using it by prepending 0x
I think there were other situation but don't remember.
I am moving away from BN anyway but as the transition happen I get these errors

This mostly happen for values and gasLimit. I agree that it is dangerous to screw up there :)

@ricmoo
Copy link
Member

ricmoo commented Sep 28, 2019

Hmmm... Actually, you have convinced me... For nonce, gasLimit, gasPrice and value, odd-length should be allowed in serializeTransaction, since those are non-ambiguously meant to be numbers.

I'll make those changes soon.

@ricmoo ricmoo added enhancement New feature or improvement. next version on-deck This Enhancement or Bug is currently being worked on. and removed discussion Questions, feedback and general information. labels Sep 28, 2019
@ricmoo
Copy link
Member

ricmoo commented Oct 19, 2019

This has been published and in the wild for a bit now, but please feel free to re-open if you have any more issues.

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement.
Projects
None yet
Development

No branches or pull requests

2 participants