[WIP] convert difficulty param to hex string #1258

scorbajio · 2021-05-17T20:58:33Z

Minimum viable example of approach to implement toHex helper function to solve issue #726 . Attempting to build and run test throws a TS2305 error. The compiler does not recognize the newly created helper function that is to be committed as a part of this PR.

packages/block/src/header-from-rpc.ts

packages/util/src/helpers.ts

ryanio · 2021-05-18T04:27:34Z

thanks for the PR! looks good, i left some notes i hope they are helpful

codecov · 2021-05-18T04:29:39Z

Codecov Report

Merging #1258 (4301902) into master (4d3edaa) will increase coverage by 0.23%.
The diff coverage is n/a.

Flag	Coverage Δ
block	`?`
blockchain	`84.42% <ø> (+0.25%)`	⬆️
client	`84.92% <ø> (+0.33%)`	⬆️
common	`88.12% <ø> (+0.04%)`	⬆️
devp2p	`83.68% <ø> (-0.60%)`	⬇️
ethash	`82.83% <ø> (+0.74%)`	⬆️
tx	`88.57% <ø> (-0.26%)`	⬇️
vm	`86.84% <ø> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

packages/block/test/from-rpc.spec.ts

packages/tx/test/base.spec.ts

packages/tx/src/baseTransaction.ts

ryanio · 2021-05-30T00:12:37Z

hey @ghorbanian, i'm sorry to lead you astray but I learned that toBuffer used to accept strings like "123" a few majors ago but was changed because they were interpreted as utf8 and would end up as the byte arrays representing that.

These things used to be true:

toBuffer("A").equals(Buffer.from([41])
toBuffer("1").equals(Buffer.from([31])

So I think it was overreaching to work on toBuffer, sorry again about that.

If you'd like to refocus scope back on just solving this at the from-rpc layer for the few specific fields that sometimes receive string integer that would be best to solve the issue at hand. I think you actually already had that code before, just not the tests to support it which you now do now and looks great.

…as integer strings

scorbajio · 2021-06-04T16:05:31Z

I made the changes in the block and util packages and placed relevant tests in from-rpc.spec.ts. Let me know what you think @ryanio.

ryanio · 2021-06-04T16:34:15Z

great, thanks, I will try to review today!

packages/block/src/from-rpc.ts

packages/block/test/from-rpc.spec.ts

ryanio · 2021-06-04T16:42:54Z

packages/util/src/index.ts

+/**
+ * Helper functions
+ */
+export * from './helpers'


i'm still not sure if we want to add this file to the exported methods, although it would be a nice catch-all for future helper methods. @holgerd77 any thoughts?

Also given this some thought.

I think we should definitely not just open up a whole new helper category here on the sideline, we've Util relatively well structured now and to introduce some "fall-back" category for things we are not sure doesn't seem like a very good idea to me.

Generally with this implementation from above we run into this problem we've discussed in the chat 1-2 weeks ago (@ghorbanian sorry, this was in our internal chat, so you just couldn't follow-up on this) that with allowing both hex-prefixed strings (being seen as hex-byte-representations) and non-hex-strings (being assumed to be numbers) there is no chance to differentiate if people want non-hex strings to be integers or (this actually happens a lot in practice) just have forgotten the 0x hex prefix but the input is actually a hex-byte-representation string.

This is really dangerous. With the regex implementation from above one is already catching a lot of stuff but just not all, and this would "break" respectively get ambiguous on strings like "1102" or the like. And it's just no win if things go wrong every 20th time or so (this is even a lot harder to debug than failures triggered every time).

I think we should at least not allow these kind of things officially in the util library when we just did the iteration of getting a lot stricter on type checking along with the v7 releases https://github.com/ethereumjs/ethereumjs-util/releases/tag/v7.0.0 for safety reasons.

Two things I can imagine here.

I think the functionality would fit a lot more naturally in the Util method suite as an intToBuffer method as a counter part to bufferToInt (in the bytes package) we already have. But this method should then really only allow integer input of various forms (integer itself, integer string, something else?). If we then want to make an additional exception for the RPC use case we should add this as custom code to the from-rpc methods in Block itself (if (hasSomeHexPrefix) use toBuffer(), else use intToBuffer)). This might be an exception worth to make in this use case since RPC responses should be relatively well defined I guess. On the Util integration side I am still not 100% sure if this aligns well with e.g. the more general toBuffer method, we likely don't want to end up with 7 different flavors of these Buffer conversion methods.

We generally hang this lower and just add this to an internal helper function in a new util.ts file in the Block library which we should then exclude from documentation in the typedoc.js file.

Atm I have a slight preference for 2., but not totally sure.

WDYGT? 🤔

Option 2 sounds like it would come with much less ambiguity. The function can keep the name integerToHex. We can think about making the helper function externally available if the need arises in the future. Any thoughts, @ryanio?

ryanio · 2021-06-04T16:52:25Z

looks like ci is failing with:

test/from-rpc.spec.ts(55,63): error TS2339: Property 'gasPrice' does not exist on type 'TypedTransaction'.
Property 'gasPrice' does not exist on type 'FeeMarketEIP1559Transaction'.

so since you know the transaction is a legacy one, you could cast it as e.g. (blockFromTransactionGasPriceAsInteger.transactions[0] as Transaction).gasPrice.toString()

come to think of it, a good follow-up PR might be to add 1559 support to from-rpc if you are up for it, otherwise it could be something I could do next week after we merge this PR :)

scorbajio · 2021-06-04T22:55:57Z

looks like ci is failing with:
test/from-rpc.spec.ts(55,63): error TS2339: Property 'gasPrice' does not exist on type 'TypedTransaction'.
Property 'gasPrice' does not exist on type 'FeeMarketEIP1559Transaction'.
so since you know the transaction is a legacy one, you could cast it as e.g. (blockFromTransactionGasPriceAsInteger.transactions[0] as Transaction).gasPrice.toString()

come to think of it, a good follow-up PR might be to add 1559 support to from-rpc if you are up for it, otherwise it could be something I could do next week after we merge this PR :)

Yea, I'd be glad to look into 1559 support next week. Thank you so much @ryanio! Would have been hard to get started without your help.

scorbajio · 2021-06-04T23:32:28Z

Any idea why I would be getting the following error every time I run tests locally from root with npm run test:

# should colorize key=value pairs
not ok 384 key=value pairs should be colorized
  ---
    operator: equal
    expected: 'test \x1b[32mkey\x1b[39m=value '
    actual:   'test key=value '
    at: Test.<anonymous> (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/packages/client/test/logging.spec.ts:31:8)
    stack: |-
      Error: key=value pairs should be colorized
          at Test.assert [as _assert] (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape/lib/test.js:228:54)
          at Test.bound [as _assert] (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape/lib/test.js:80:32)
          at Test.equal (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape/lib/test.js:389:10)
          at Test.bound (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape/lib/test.js:80:32)
          at Test.<anonymous> (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/packages/client/test/logging.spec.ts:31:8)
          at Test.bound [as _cb] (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape/lib/test.js:80:32)
          at Test.exports.Test.run (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape-catch/index.js:26:10)
          at Test.bound [as run] (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape/lib/test.js:80:32)
          at Test._end (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape/lib/test.js:165:11)
          at Test.exports.Test._end (/home/indigoomega021/code/projects/ethereumjs/ethereumjs-monorepo/node_modules/tape-catch/index.js:16:8)
  ...

I did some digging and I think it only fails locally. I just force push with --no-verify and CI/CD tests don't catch it at any point.

ryanio · 2021-06-04T23:58:36Z

hm no need to worry about that, maybe you are using a terminal without color support

scorbajio mentioned this pull request May 17, 2021

block: Check and convert difficulty param to hex string #1254

Closed

ryanio reviewed May 18, 2021

View reviewed changes

packages/block/src/header-from-rpc.ts Outdated Show resolved Hide resolved

ryanio reviewed May 18, 2021

View reviewed changes

packages/util/src/helpers.ts Outdated Show resolved Hide resolved

ryanio reviewed May 18, 2021

View reviewed changes

packages/util/src/helpers.ts Outdated Show resolved Hide resolved

ryanio reviewed May 18, 2021

View reviewed changes

packages/util/src/helpers.ts Outdated Show resolved Hide resolved

scorbajio force-pushed the number-to-hex-conversion branch from c48db7e to 48405bc Compare May 19, 2021 23:23

ryanio reviewed May 20, 2021

View reviewed changes

packages/block/test/from-rpc.spec.ts Outdated Show resolved Hide resolved

ryanio reviewed May 20, 2021

View reviewed changes

packages/tx/test/base.spec.ts Outdated Show resolved Hide resolved

ryanio reviewed May 20, 2021

View reviewed changes

packages/tx/src/baseTransaction.ts Outdated Show resolved Hide resolved

scorbajio force-pushed the number-to-hex-conversion branch from 3c30264 to 5f59124 Compare May 22, 2021 22:03

scorbajio requested a review from ryanio May 29, 2021 20:34

scorbajio added 2 commits June 4, 2021 12:01

util: add numberToHex helper method

5815040

block: handle difficulty, gasPrice, and value parameters being given …

4301902

…as integer strings

scorbajio force-pushed the number-to-hex-conversion branch from 8de05a3 to 4301902 Compare June 4, 2021 16:03

ryanio reviewed Jun 4, 2021

View reviewed changes

packages/block/src/from-rpc.ts Show resolved Hide resolved

ryanio reviewed Jun 4, 2021

View reviewed changes

packages/block/test/from-rpc.spec.ts Show resolved Hide resolved

ryanio reviewed Jun 4, 2021

View reviewed changes

ryanio added package: block package: util labels Jun 5, 2021

scorbajio mentioned this pull request Jun 23, 2021

block: check and convert difficulty, gasPrice, and value parameters if string integers in from-rpc #1316

Merged

scorbajio closed this Jun 23, 2021

This was referenced Aug 20, 2023

[Snyk] Upgrade lint-staged from 13.0.3 to 13.2.3 Woodpile37/ethereumjs-monorepo#9

Merged

[Snyk] Upgrade lint-staged from 13.0.3 to 13.2.3 Woodpile37/ethereumjs-monorepo#42

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] convert difficulty param to hex string #1258

[WIP] convert difficulty param to hex string #1258

scorbajio commented May 17, 2021 •

edited

Loading

ryanio commented May 18, 2021

codecov bot commented May 18, 2021 •

edited

Loading

ryanio commented May 30, 2021

scorbajio commented Jun 4, 2021

ryanio commented Jun 4, 2021 •

edited

Loading

ryanio Jun 4, 2021

holgerd77 Jun 7, 2021 •

edited

Loading

scorbajio Jun 12, 2021

ryanio commented Jun 4, 2021

scorbajio commented Jun 4, 2021 •

edited

Loading

scorbajio commented Jun 4, 2021

ryanio commented Jun 4, 2021

[WIP] convert difficulty param to hex string #1258

[WIP] convert difficulty param to hex string #1258

Conversation

scorbajio commented May 17, 2021 • edited Loading

ryanio commented May 18, 2021

codecov bot commented May 18, 2021 • edited Loading

Codecov Report

ryanio commented May 30, 2021

scorbajio commented Jun 4, 2021

ryanio commented Jun 4, 2021 • edited Loading

ryanio Jun 4, 2021

Choose a reason for hiding this comment

holgerd77 Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

scorbajio Jun 12, 2021

Choose a reason for hiding this comment

ryanio commented Jun 4, 2021

scorbajio commented Jun 4, 2021 • edited Loading

scorbajio commented Jun 4, 2021

ryanio commented Jun 4, 2021

scorbajio commented May 17, 2021 •

edited

Loading

codecov bot commented May 18, 2021 •

edited

Loading

ryanio commented Jun 4, 2021 •

edited

Loading

holgerd77 Jun 7, 2021 •

edited

Loading

scorbajio commented Jun 4, 2021 •

edited

Loading