Skip to content
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

Merged
merged 24 commits into from
Sep 2, 2024
Merged

feat: add decoding methods #10

merged 24 commits into from
Sep 2, 2024

Conversation

NazariiDenha
Copy link
Contributor

@NazariiDenha NazariiDenha commented Jun 11, 2024

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 DA

PR 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:

  • build: Changes that affect the build system or external dependencies (example scopes: yarn, eslint, typescript)
  • ci: Changes to our CI configuration files and scripts (example scopes: vercel, github, cypress)
  • docs: Documentation-only changes
  • feat: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that doesn't fix a bug, or add a feature, or improves performance
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • test: Adding missing tests or correcting existing tests

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change
  • Yes

@NazariiDenha
Copy link
Contributor Author

I will also add some tests later

@Thegaram Thegaram changed the title feat: add decoding mehods feat: add decoding methods Jun 12, 2024
@roynalnaruto roynalnaruto self-requested a review June 12, 2024 15:18
@NazariiDenha
Copy link
Contributor Author

NazariiDenha commented Jun 12, 2024

I will add decoding for codecv2 and then will be ready for review

@NazariiDenha
Copy link
Contributor Author

ready for review

encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
encoding/codecv0/codecv0.go Outdated Show resolved Hide resolved
encoding/codecv0/codecv0.go Outdated Show resolved Hide resolved
encoding/codecv3/codecv3.go Outdated Show resolved Hide resolved
encoding/codecv3/codecv3.go Outdated Show resolved Hide resolved
encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
encoding/codecv1/codecv1.go Outdated Show resolved Hide resolved
encoding/codecv1/codecv1.go Outdated Show resolved Hide resolved
encoding/codecv1/codecv1.go Outdated Show resolved Hide resolved
encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
encoding/codecv1/codecv1.go Outdated Show resolved Hide resolved
Copy link
Contributor

@roynalnaruto roynalnaruto left a 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

encoding/codecv0/codecv0.go Show resolved Hide resolved
encoding/codecv0/codecv0.go Show resolved Hide resolved
encoding/codecv0/codecv0.go Outdated Show resolved Hide resolved
encoding/codecv0/codecv0.go Outdated Show resolved Hide resolved
encoding/codecv0/codecv0.go Outdated Show resolved Hide resolved
encoding/codecv1/codecv1.go Show resolved Hide resolved
encoding/codecv1/codecv1.go Outdated Show resolved Hide resolved
encoding/codecv1/codecv1.go Show resolved Hide resolved
encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
jonastheis
jonastheis previously approved these changes Aug 27, 2024
@jonastheis
Copy link
Contributor

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

Copy link
Contributor

@roynalnaruto roynalnaruto left a 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.

encoding/codecv4/codecv4.go Outdated Show resolved Hide resolved
encoding/codecv4/codecv4.go Outdated Show resolved Hide resolved
encoding/da.go Outdated Show resolved Hide resolved
encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
encoding/codecv2/codecv2.go Outdated Show resolved Hide resolved
encoding/codecv1/codecv1.go Outdated Show resolved Hide resolved
@0xmountaintop
Copy link
Member

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?

@colinlyguo
Copy link
Member

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.

@colinlyguo
Copy link
Member

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

agree.

@NazariiDenha NazariiDenha merged commit 41c6486 into main Sep 2, 2024
3 checks passed
@0xmountaintop 0xmountaintop deleted the feat/add-decoding-methods branch September 3, 2024 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants