[Cleanup] Initial documentation improvements and code cleanup #8

h5law · 2023-05-25T12:04:12Z

Overview

This PR addresses the lack of documentation in the repo by adding some details to the README.md file and adds some more godoc comments throughout the codebase.

The workflow files have been updated to a use go version 1.18 and have been updated to fully test the repo and produce code coverage reports and check against linters.

The legacy fuzzing has been removed and instead now uses go native fuzzing, the fuzzing tests remain the same and can be improved upon in a separate PR. The oss-fuzz build script has been removed until this repo is on oss-fuzz a new script will not be made.

Additionally linter errors have been addressed.

Issue

Fixes #7

Additional testing enhancements will be covered in a separate PR

Checklist

Update any relevant README(s)
Add or update any relevant or supporting mermaid diagrams
Task specific tests or benchmarks: go test ...
New tests or benchmarks: go test ...
All tests: go test -v

reviewpad · 2023-05-25T12:28:08Z

Reviewpad Report (Reviewpad ran in dry-run mode because configuration has changed)

📜 Executed actions

$removeLabel("small")
$removeLabel("medium")
$addLabel("large")
$removeLabel("documentation")
$removeLabel("dependencies")

…t until package is on OSS-Fuzz

fuzz_test.go

…ly after each op

codecov-commenter · 2023-05-26T00:11:43Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (5c4037e) 84.53% compared to head (0931fa5) 84.53%.

❗ Current head 0931fa5 differs from pull request most recent head 86b4235. Consider uploading reports for the commit 86b4235 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main       #8   +/-   ##
=======================================
  Coverage   84.53%   84.53%           
=======================================
  Files           8        8           
  Lines         776      776           
=======================================
  Hits          656      656           
  Misses         85       85           
  Partials       35       35

Impacted Files	Coverage Δ
hasher.go	`100.00% <ø> (ø)`
mapstore.go	`90.47% <ø> (ø)`
options.go	`50.00% <ø> (ø)`
smt.go	`80.91% <ø> (ø)`
types.go	`87.23% <ø> (ø)`
smt_testutil.go	`61.70% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…matic way

.github/workflows/test.yml

README.md

Olshansk · 2023-05-30T18:55:02Z

types.go

@@ -12,6 +12,7 @@ const (
 var (
 	defaultValue []byte = nil

+	// ErrKeyNotPresent is returned when a key is not present in the tree.
 	ErrKeyNotPresent = errors.New("key already empty")


I feel like "key already empty" should be an error for trying to delete an unavailable leaf. ErrKeyNotPresent could also be used when we try to retrieve something non-existent, so it feels like we're conflating a couple things.

So this error is actually only used in the Delete function as if we try to get a key that's not set it returns an empty value - bc all keys are technically set but most are just empty.

smt.go

Olshansk · 2023-05-30T19:01:37Z

smt.go

@@ -6,8 +6,8 @@ import (
 )

 var (
-	_ treeNode = &innerNode{}
-	_ treeNode = &leafNode{}
+	_ treeNode = (*innerNode)(nil)


Was this a recommendation of the linter?

I'm personally a bigger fan of the previous approach, or potentially using var _ treeNode = new(innerNode) instead

I actually thought about this a lot and went into the golang discord and this is the idiomatic go way to do this.

There is potentially some issues around the other way linked here but it is unclear if this affects this specific use case.

README.md

fuzz_test.go

Co-authored-by: Daniel Olshansky <[email protected]>

README.md

Olshansk

Let's keep iterating (merkle sum, radix tree, visualization tooling etc...) and improving the docs, but this, as is, is already really solid work.

Here's to moving the industry forward 🥂

README.md

Olshansk · 2023-05-31T21:43:58Z

.github/PULL_REQUEST_TEMPLATE/pull_request_template.md

@@ -0,0 +1,65 @@
+<!-- REMOVE this comment block after following the instructions


I realize the urge to throw small things like this into larger PRs, but do try to separate it in the future. nbd this time

Olshansk · 2023-06-01T01:30:35Z

README.md

+
+The different nodes types described above make the tree have a structure similar to the following:
+
+```mermaid


I think you made really good progress on this, but there is still a lot of opportunity to improve it so it's easier for the reader to understand & interpret. Similar to what the JMT and Relay Mining Paper do. For example, by showing (k, v) = (a, b) or path=0b001 or path_bounds=....

In either case, the visualizer we build might do this automatically for us in the future (we can just have a python structure to mermaid converter) so not a blocker for now.

https://developers.diem.com/papers/jellyfish-merkle-tree/2021-01-14.pdf
https://arxiv.org/pdf/2305.10672.pdf

I have added an interim diagram before we can have a more detailed one to show the paths of different nodes

README.md

Olshansk · 2023-06-01T01:33:35Z

README.md

+
+_Note_: This diagram is not entirely accurate regarding the process of creating a leaf node, but is a good representation of the process.
+
+```mermaid


Amazing diagram! 👏

Co-authored-by: Daniel Olshansky <[email protected]>

h5law and others added 8 commits May 24, 2023 11:44

Update readme and godoc comments

11458c2

Update README

8b80a0c

Update README.md

f315383

Update workflow file

0e5d4d0

Update fuzz workflow

db8d51b

Fix workflow

0cc5f71

Update workflow name

3a72bcf

Address linter errors

eb1d684

h5law added the documentation Improvements or additions to documentation label May 25, 2023

h5law self-assigned this May 25, 2023

reviewpad bot requested a review from Olshansk May 25, 2023 12:04

reviewpad bot added medium Pull request is medium waiting-for-review This PR is currently waiting to be reviewed and removed documentation Improvements or additions to documentation labels May 25, 2023

h5law added 2 commits May 25, 2023 13:10

Add coverage file to codecov upload

ad3f7c1

Merge branch 'main' into documentation_cleanup

de15fba

reviewpad bot added large Pull request is large and removed medium Pull request is medium labels May 25, 2023

Update reviewpad workflows to remove unneeded jobs

671b17e

pokt-network deleted a comment from reviewpad bot May 25, 2023

h5law added 2 commits May 25, 2023 13:24

Remove executable permissions as for remote build on oss-fuzz

c3bb4f2

Change if statements in test workflow jobs

5d396be

pokt-network deleted a comment from reviewpad bot May 25, 2023

h5law added 3 commits May 25, 2023 13:32

Revert if statement changes

1951c99

Add missing env var to workflow

d663d97

Panic on errors in fuzz tests

c2882dc

h5law added 2 commits May 25, 2023 16:25

Remove uneeded field call

32a3085

Remove legacy fuzzes with go native fuzz, remove OSS-Fuzz build scrip…

22c938b

…t until package is on OSS-Fuzz

Olshansk requested changes May 25, 2023

View reviewed changes

Add comments to fuzz test and check root hash is behaving appropriate…

1d2fb80

…ly after each op

h5law requested a review from Olshansk May 26, 2023 10:51

This was referenced May 26, 2023

[Persistence] Integrate lazy loading SMT changes into V1 pokt-network/pocket#790

Merged

[Testing] Enhance overall testing in the repo #9

Closed

h5law added 2 commits May 28, 2023 11:08

Fix codecov upload file format

ed8f9d7

Revert to nil pointer interface implementation as this is the go idio…

353c30f

…matic way

Olshansk requested changes May 30, 2023

View reviewed changes

h5law and others added 6 commits May 30, 2023 21:59

Update fuzz_test.go

887969c

Co-authored-by: Daniel Olshansky <[email protected]>

Update smt.go

6e6d543

Co-authored-by: Daniel Olshansky <[email protected]>

Update fuzz_test.go

2f16a8b

Co-authored-by: Daniel Olshansky <[email protected]>

Update fuzz_test.go

b16f0f9

Co-authored-by: Daniel Olshansky <[email protected]>

Clarify comment

9538bba

Add more explanations aroud node types, paths, hashers and digests

cfada84

h5law requested a review from Olshansk May 30, 2023 22:58

h5law added 2 commits May 30, 2023 23:59

Add line break between note

7403924

Clarify node storage

5581c04

Olshansk reviewed May 30, 2023

View reviewed changes

README.md Show resolved Hide resolved

Update workflow to use go 1.18

91efe8f

h5law requested a review from Olshansk May 31, 2023 08:56

h5law added 2 commits May 31, 2023 10:44

Add PR Template file

0965b0a

Add go report card and change workflow title

0931fa5

Olshansk approved these changes Jun 1, 2023

View reviewed changes

h5law and others added 2 commits June 1, 2023 09:59

Update README.md

51acc15

Co-authored-by: Daniel Olshansky <[email protected]>

Add path visualisation

86b4235

h5law merged commit cc555d9 into main Jun 1, 2023

h5law deleted the documentation_cleanup branch June 1, 2023 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cleanup] Initial documentation improvements and code cleanup #8

[Cleanup] Initial documentation improvements and code cleanup #8

h5law commented May 25, 2023 •

edited

Loading

reviewpad bot commented May 25, 2023 •

edited

Loading

codecov-commenter commented May 26, 2023 •

edited by codecov bot

Loading

Olshansk May 30, 2023

h5law May 30, 2023

Olshansk May 30, 2023

h5law May 30, 2023

Olshansk left a comment

Olshansk May 31, 2023

Olshansk Jun 1, 2023

h5law Jun 1, 2023

Olshansk Jun 1, 2023

		@@ -0,0 +1,65 @@
		<!-- REMOVE this comment block after following the instructions


		The different nodes types described above make the tree have a structure similar to the following:

		```mermaid


		_Note_: This diagram is not entirely accurate regarding the process of creating a leaf node, but is a good representation of the process.

		```mermaid

[Cleanup] Initial documentation improvements and code cleanup #8

[Cleanup] Initial documentation improvements and code cleanup #8

Conversation

h5law commented May 25, 2023 • edited Loading

Overview

Issue

Checklist

reviewpad bot commented May 25, 2023 • edited Loading

codecov-commenter commented May 26, 2023 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h5law commented May 25, 2023 •

edited

Loading

reviewpad bot commented May 25, 2023 •

edited

Loading

codecov-commenter commented May 26, 2023 •

edited by codecov bot

Loading