-
-
Notifications
You must be signed in to change notification settings - Fork 410
feat: add Era File Reading and Writing #8035
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
base: unstable
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we wanna add types separately? A more complete implementation makes more sense to me before reviewing
|
@nflaig I had a chat with @wemeetagain where we agreed to proceed like that https://discord.com/channels/593655374469660673/1387128551962050751/1388555043921461300 |
I agree with the part that we don't need to integrate it with the beacon node in the first iteration but just having the types without any actual code imo makes not much sense. I'd prefer a more complete implementation but leaving this up to @wemeetagain |
|
@nflaig while implementing, i found some small optimisations that deviated from the spec. So I may have to change the types later on. I think it would be better if I add the reading and writing of era files in this pr too so that we can consolidate on the major types we need. |
|
Yeah go for it. It will be easier to review the types if we can see how they're being used. |
|
I modified the pr to focus on era file reading and testing it with a script on .era file from https://mainnet.era.nimbus.team/ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8035 +/- ##
============================================
- Coverage 51.95% 51.91% -0.05%
============================================
Files 848 851 +3
Lines 65941 66096 +155
Branches 4814 4816 +2
============================================
+ Hits 34258 34312 +54
- Misses 31615 31715 +100
- Partials 68 69 +1 🚀 New features to boost your workflow:
|
|
not quite sure why The CI/CD is showing test timed out in places where I have not touched , but I got this test time timed out issues on previous runs too. What is the root cause for those errors? |
…e cyclic dependency
…into add_types_era
packages/reqresp/src/encodingStrategies/sszSnappy/snappyFrames/compress.ts
Show resolved
Hide resolved
|
@guha-rahul did you go through the gemini bot comments? feel free to resolve them if it's not a good suggestions / not applicable, you can also add a comment to it why you think it's wrong what the bot suggests |
@nflaig Yes, all of them have been worked out(Some by cayman and some by me) except the one with console logs since for testing they are quite hellpful and I plan to remove them when its ready to be merged. Also should i remove the snappy code from utils package to packages/reqresp/utils? |
feel free to resolve them in that case if it's already addressed so we have a better overview of what's missing so we can merge this |
done! |
packages/era/test/e2e-mainnet/era.readwrite.integration.test.ts
Outdated
Show resolved
Hide resolved
|
docs checks are failing, see job run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Motivation
Description
cd packages/era/test && ./download_era_file.sh)test:e2eis not defined for the era package)