-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: add decoding methods #10
Conversation
I will also add some tests later |
ca7ce87
to
a523bce
Compare
a523bce
to
7665c1e
Compare
83fbd77
to
129fde6
Compare
I will add decoding for codecv2 and then will be ready for review |
ready for review |
…-codec into feat/add-decoding-methods
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.
Some refactoring suggestions, questions and request for more inline documentation
Let's get this merged and then integrate into #25. Which means we need to revisit the implementation anyway. Therefore, I think this PR doesn't need to be perfect |
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.
A few minor suggestions and nit-picks.
This PR is tested with scroll-tech/go-ethereum#1013, @colinlyguo do you have time to review this PR so that we can merge it? |
LGTM. and approved. |
agree. |
Purpose or design rationale of this PR
Describe your change. Make sure to answer these three questions: What does this PR do? Why does it do it? How does it do it?
Implement some decoding method, that will be used by
l2geth
during the process of syncing from DAPR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
Breaking change label
Does this PR have the
breaking-change
label?