Closed jochem-brouwer closed 3 years ago
Merging #290 (be76516) into master (fe518cf) will increase coverage by
0.02%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #290 +/- ##
==========================================
+ Coverage 97.10% 97.13% +0.02%
==========================================
Files 11 11
Lines 415 454 +39
Branches 71 79 +8
==========================================
+ Hits 403 441 +38
Misses 3 3
- Partials 9 10 +1
Impacted Files | Coverage Ξ | |
---|---|---|
src/account.ts | 94.64% <100.00%> (+0.14%) |
:arrow_up: |
src/bytes.ts | 97.46% <100.00%> (ΓΈ) |
|
src/signature.ts | 93.75% <100.00%> (-2.41%) |
:arrow_down: |
src/types.ts | 100.00% <100.00%> (+20.00%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Ξ = absolute <relative> (impact)
,ΓΈ = not affected
,? = missing data
Powered by Codecov. Last update fe518cf...46415c1. Read the comment docs.
Really curious about this PR. Is this because of EIP155? Some chainIDs don't fit in a number, so computing the V requires a big number?
@alcuadrado Yep, this was a problem on the YoloV3 network, which has a chain id of 34180983699157880
. If we don't use the logic as in this PR, then at some point when syncing, we recover the wrong sender address (probably because some rounding errors in the arithmetic).
I will add this transaction as test.
You know what's extra curious about it? In most platforms that's not a BN! It fits in a uint64, but js π₯²
Okay, isolated the failing transaction on Yolo and added it as test. Ready for review.
Yes, also thought after writing that we likely should leave BigInt
out for now and extract to a separate issue.
Not sure, I know we want to get this released soon since the client work depends on this, on the other hand we might want to also adopt the other methods as well which are touched by this since this would better justify a release and I think this is something others would directly profit from.
Have written together a short list to get an overview on the task:
toChecksumAddress = function(hexAddress: string, eip1191ChainId?: number)
toChecksumAddress
ecsign = function(msgHash: Buffer, privateKey: Buffer, chainId?: number)
chainId
is BN
) (a bit tricky)toRpcSig = function(v: number, r: Buffer, s: Buffer, chainId?: number): string
(v
and chainId
)
v
and chainId
)
I think this should be doable, WDYT? π€ If you want you can directly integrate here but I could also do tomorrow if you don't find the extra time.
For the function overloading cases I guess the most clean way of doing this is likely to always return the same type as the input type? This would then be slightly more work, but I guess it's better than always returning a BN
when people are e.g. working in a Buffer context.
Monorepo integration: @ryanio has also already suggested this and therefore already did #289 in preparation. π I don't have the capacity right now to do the integration and I also feel that the monorepo is not enough on stable ground right now to do such a heavy integration on top (CI/Coverage often failing and things). But this is definitely planned! π
(or @ryanio you can also directly pick this up if you want to! π )
Yeah this should not be so hard. So what is the plan here? For anything where it would make sense to allow Buffer/BN/string/number
input we allow these? How do we internally want to handle this? I assume we will convert them all to a BN
or a Buffer
, depending on the situation?
In future PRs we can start to use BigInt
or Uint8Arrays
(as opposed to Buffer
, for optimization)
Handling BNLike
would be nice from a dev ux perspective, like we do in the fromXData
factories in our monorepo packages. Maybe we can write some kind of utility/formatter to consume BNLike
and output a specific type (BN
or Buffer
/Uint8Array
) so it can be reused in other places.
Yeah this should not be so hard. So what is the plan here? For anything where it would make sense to allow
Buffer/BN/string/number
input we allow these? How do we internally want to handle this? I assume we will convert them all to aBN
or aBuffer
, depending on the situation?
I think it makes sense to on this first round stay somewhat narrow here and not do all methods somehow accepting numbers, so e.g. for many of the buffer-related methods (setLengthLeft()
,...) I would doubt that we need BN
s there, or at least that would likely need some discussion. But on a first look I would say that these v
respectively chainId
related methods are safe candidates to do. So I would suggest we stick to the (I think complete in this regard) list from above.
Yes, I think in most cases, an internal conversion to BN
should work well - e.g. for toChecksumAddress
and similar methods. Not completely sure about the overload methods - e.g. ecsign
- might be most practical to stay in the respective type if we choose a BN
in-and-out, Buffer
-in-and-out,... methodology, but this will likely becoming more clear when working on this.
Phew, this is a lot harder than I expected. Already had various iterations on this, I started by expanding the ecsign
method. Will nevertheless continue to work here since I do think that we need this anyhow if we e.g. want to expand on PoA block signing and the like. And this is generally useful to have as a feature.
BNLike
type both as an input type as well as a return type. This is not so beautiful since it reduces on type safety for the return type and also directly breaks typescript code which assumed before to get a number backBN
: but this is not so beautiful from a future BigInt
support perspectiveecrecover
method: the right thing to do here
ECDSASignatureBuffer
ecrecover
- which can be easily converted to BN
or `BigIntECDSASignatureBuffer
(this will be renamed to ECDSASignatureBuffer
) as deprecated and switch to always returning a Buffer on the next breaking releaseSo I will introduce 4. on the next push.
Hmpfg.
Ok, this 24279bf839eb697a8f8e2b643662f6d2986c266b is finally how I would keep it for this specific function, will continue with the other methods today or tomorrow.
Have added a planned-breaking-change note to https://github.com/ethereumjs/ethereumjs-util/issues/291
Ok, this is now ready for review. π
Have left out fromRpcSig()
- from the doc comments I have the impression that this method is outdated and we can deprecate? Not sure about toRpcSig()
, have adopted for now.
Otherwise I think the main v and chainId related functionality should be covered.
Side note: first planned to also include a release commit to get this out quicker (PR is needed for continued client work), but since this has now gotten so large I've omitted to not overload.
(or does someone feel uneasy here with opening to so many type inputs? we can alternatively also just limit to (the existing) number + Buffer)
(wonder since we have lately rather reduced type variety)
(I have some (but no strong) tendency to think that in this case it is ok)
looks great, however instead of converting the types inside each method (seems repetitive) what if we introduced a helper method e.g. toType(from: any, to: TypeOutput.Buffer)
where TypeOutput
is an enum of [Number, Buffer, BN]
(in the future can add BigInt
)
Then we can also refactor out vAndChainIdTypeChecks
and include the 0x-prefix and MAX_SAFE_INTEGER checks inside toType
if it is given a number or string, so we automatically have that safety everywhere.
Hi @ryanio can you eventually finish this PR? I won't be able to finish today or tomorrow and would be nice if we get this done, I'm fine with all changes. π
Hi @ryanio can you eventually finish this PR? I won't be able to finish today or tomorrow and would be nice if we get this done, I'm fine with all changes. π
yes for sure I can do in a few hours when I'm back on my computer π
(so finish === include eventual improvement suggestions, otherwise this should be mostly ready)
ok I added a commit (https://github.com/ethereumjs/ethereumjs-util/pull/290/commits/243fd4776d789c7b8c37dc22d0249465d1a0766c) that adds a toType
helper that includes the 0x-prefix and MAX_SAFE_INTEGER checks so I was able to reduce some of that duplicate code, let me know what you guys think.
I would appreciate some TypeScript guru help, I was able to get the toType
return type based on the outputType
enum with some fancy code here, however I wasn't able to get the return values inside the method to work seamlessly (I had to add as any
to all the returns). Otherwise when using the toType
method in the actual code as a helper it seems to be reporting the correct return type so that's exciting!
I might also want to add some independent tests for toType
if we will be exporting it for use by consumers of the package, but I can do that in a follow-up PR if we want to merge this in. (edit: added in https://github.com/ethereumjs/ethereumjs-util/pull/290/commits/88db47f494b531fdcef3e54dd3030aa3066af0e8)
(BTW @holgerd77, do you think it was okay to refactor out the "Performance optimization" here? I can add it back in, but opted for the new simplicity and built in checks introduced by the toType
helper.)
In https://github.com/ethereumjs/ethereumjs-monorepo/pull/1129 I noticed that
ecrecover
does not have support for very high recovery IDs (which happen on very high chain IDs). This PR allows to pass BNs or Buffers to ecrecover, to allow this support.I will add a test to show that this works for high chain IDs by taking a YoloV3 transaction.