Common: added static method for checking if chainId is supported #1281
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
| ) | ||
| st.end() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Thanks for the PR! 😄
Two things here:
- We currently do not organize the tests in the libraries on that granularity level of having one file per method and so this test file would jump somewhat out of the line, can you please add to one of the existing files? I guess
chains.spec.tsshould be a good candidate. Not that we can't reorg at some PIT if we see the need, but these kind of things should generally be discussed before. - Can you also add a positive test case here for a chain ID existing?
There was a problem hiding this comment.
@holgerd77 Thanks for the review!
Re: 1, 👍. I wasn't sure what existing file it should go in so I made a new one, but your point makes sense. I'll move the test into chains.spec.ts.
Re: 2, I think I have that already here (https://github.com/ethereumjs/ethereumjs-monorepo/pull/1281/files#diff-00e881872be66da72fe4e5df60785751bb9b0fce068df279a7fd63b9dcdd0423R7), unless I'm not understanding? Actually now that I look at it, it is a little awkward having the top-level test be should return the correct response. I can break them out into separate tests.
There was a problem hiding this comment.
Also, I had a question unrelated to my code changes, just something I noticed. There's a file in packages/common/tests named customChains.ts. Should the file name be customChains.spec.ts?
There was a problem hiding this comment.
@emersonmacro oh yes, good catch. 😄 We have a collecting issue open here where we collect some of the smaller fixes to then be worked on in batch (or along some other PRs) #905, have added there so that it won't get lost.
Addresses #1243. Feedback welcome!