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

Initial Review of Pocket Prunner #1

Merged
merged 4 commits into from
Jul 14, 2023
Merged

Initial Review of Pocket Prunner #1

merged 4 commits into from
Jul 14, 2023

Conversation

Olshansk
Copy link
Contributor

Primarily needs and edits across README, main.go and appdb.go

My keynotes are:

  1. Improvement: Lack of documentation of how it works
  2. Improvement: Lack of comments to help the reader understand
  3. Confusing: Terminology like “version” which I presume is height
  4. Important Question: We moved away from amino in 2022, so how are we still using it everywhere?
  5. Documentation: Can you add some documentation in verifier.go on details on how it works?
  6. Please pay attention to the DISCUSS_IN_THIS_PR comments in particular

Regarding code review:

  1. If you give me access to this repo, I can cherry-pick these changes to a branch in your repo
  2. Happy to review/merge this in and let you iterate on top of it
    Let me know!

@msmania
Copy link
Owner

msmania commented Jul 14, 2023

Thank you for your thorough review! I'll merge this and push follow-up commits later. We think documentation part is not a ship-blocker, so I focus on addressing code comments so that we can announce official release while I'm working on documentation.

  1. Improvement: Lack of documentation of how it works

Will add it later (probably as a separate markdown file). #3

  1. Confusing: Terminology like “version” which I presume is height

It's actually called "version" in some packages of pocket-core such as iavl or rootmulti.

  1. Important Question: We moved away from amino in 2022, so how are we still using it everywhere?

Amino is still used in the iavl store. See node.go.

  1. Documentation: Can you add some documentation in verifier.go on details on how it works?

This is a feature for dev purpose. I'll add a new section to README.md. #2

  1. Please pay attention to the DISCUSS_IN_THIS_PR comments in particular

Thank you for clarifying these in the code. I'll merge this PR and take care of them.

@msmania msmania merged commit 88934a2 into msmania:main Jul 14, 2023
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.

2 participants