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

[Code Health] refactor: SMST#Root(), #Sum(), & #Count() #51

Merged
merged 10 commits into from
Jul 17, 2024

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Jul 12, 2024

Summary

  • Add:
    • MerkleSumRoot
    • SMST#MustCount()
    • SMST#MustSum()
    • MerkleSumRoot#MustCount()
    • MerkleSumRoot#DigestSize()
    • MerkleSumRoot#HasDigestSize()
  • Refactor:
    • SMST#Root() to return MerkleSumRoot

Human Summary

AI Summary

reviewpad:summary

Issue

Fixes pokt-network/poktroll#584

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make test_all
  • Run all/relevant benchmarks (if optimising): make benchmark_{all | suite name}

Required Checklist

If Applicable Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated any relevant README(s)/documentation and left TODOs throughout the codebase
  • Add or update any relevant or supporting mermaid diagrams

@bryanchriswhite bryanchriswhite added the code health Related to code cleanup and health of the repo label Jul 12, 2024
@bryanchriswhite bryanchriswhite self-assigned this Jul 12, 2024
root.go Outdated Show resolved Hide resolved
root.go Outdated Show resolved Hide resolved
smst.go Outdated Show resolved Hide resolved
Signed-off-by: Bryan White <[email protected]>
@bryanchriswhite bryanchriswhite marked this pull request as ready for review July 12, 2024 11:03
Copy link
Contributor

@red-0ne red-0ne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small NIT comment.

Also smst_test.go seems to use require.Equal(t, uint64(1), impl.Count()) where it should use impl.MustCount().

root.go Outdated Show resolved Hide resolved
root.go Outdated Show resolved Hide resolved
root.go Outdated Show resolved Hide resolved
var countBz [countSizeBytes]byte
copy(countBz[:], []byte(r)[firstCountByteIdx:])
return binary.BigEndian.Uint64(countBz[:])
// validateBasic returns an error if the root digest size is not a power of two.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Move this comment INSIDE the function and keep the godoc more general OR call out a power of two as an explicit example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm either not quite following or am just unable to think of an alternative godoc comment that satisfies your suggestion. Do you have a concrete example in mind (i.e., validateBasic returns an error if ...)?

@bryanchriswhite bryanchriswhite merged commit 3de80fe into main Jul 17, 2024
2 checks passed
@bryanchriswhite bryanchriswhite deleted the refactor/smst/sum-count branch July 17, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Related to code cleanup and health of the repo
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[TODO] @Olshansk's (and now @Bryan's) Blocker TODOs
3 participants