Fd 1211 factable solutions updates october #901

stackdump · 2019-10-18T14:00:14Z

Merge fact able Oct changes into a4

sync with their actual values

changes. Calling init() and refactor an else statement

and EC_CHAINID_STRING Comments and receiver changes, and else refactorings

refactor init() call and 'else' statements, delete confusing and mostly duplicative function which is never called outside this file

Fd 1211 factable branch

WhoSoup

hope you don't mind but I took the liberty of providing some feedback. there are some ambiguities and some mistakes, as well as some things that could be improved

overall, i think there is a pattern of over-commenting things, particularly of the struct fields (see my "redundant" comments). comments should add extraneous information that you can't determine from just reading the code, or explain difficult to understand processes, or summarize things. just paraphrasing the name of the variable doesn't do anything and is just more work for you

WhoSoup · 2019-10-20T09:03:34Z

common/constants/runstate/runStates.go

 )

-// Returns if factomd is terminated or in the process of terminating
+// IsTerminating returns true if factomd is terminated or in the process of terminating


is this function a misnomer? it turns a hard > Stopping. the only one that passes is "Stopped". func should be called IsTerminated imo, or we change this to a >=. the only place it's used is in missing message requests, quitting those earlier shouldn't be a problem

WhoSoup · 2019-10-20T09:04:03Z

common/constants/runstate/runStates.go

-	Running  RunState = 2
-	Stopping RunState = 3
-	Stopped  RunState = 4
+	New      RunState = 0 // State of a newly created Factomd object


this can be implement as an iota

WhoSoup · 2019-10-20T09:08:14Z

common/entryBlock/eblock.go

@@ -15,10 +15,10 @@ import (

 // EBlock is the Entry Block. It holds the hashes of the Entries and its Merkle
 // Root is written into the Directory Blocks. Each Entry Block represents all
-// of the entries for a particular Chain during a 10 minute period.
+// of the entries for a particular Chain during a 10 minute period (a single directory block's worth of time)


Unfortunately, there is some really ambiguous language prevalent throughout these comments and it would be great to clear up some of the confusion.

The first thing is we should distinguish between "eblock entries" and "chain entries". "eblock entries" are the values stored in the eblock's "body" and are 32-byte sized objects that are either a) the entry hash of a chain entry, or b) an end-of-minute marker.

I think this comment should be changed to:

// EBlock (Entry Block) holds an index of all entries written to a particular chain in the course of a Directory Block.
// The EBlock's Merkle Root is written to the Directory Block.

WhoSoup · 2019-10-20T09:11:50Z

common/entryBlock/eblock.go

 type EBlock struct {
-	Header interfaces.IEntryBlockHeader `json:"header"`
-	Body   *EBlockBody                  `json:"body"`
+	Header interfaces.IEntryBlockHeader `json:"header"` // Header of the Eblock


this comment is redundant and adds nothing useful

WhoSoup · 2019-10-20T09:13:46Z

common/entryBlock/eblock.go

-	Header interfaces.IEntryBlockHeader `json:"header"`
-	Body   *EBlockBody                  `json:"body"`
+	Header interfaces.IEntryBlockHeader `json:"header"` // Header of the Eblock
+	Body   *EBlockBody                  `json:"body"`   // Array of entries from a single chain id associated with this entry block


two things:

it's a slice not an array

with respect to my comment above, this should be "eblock entries". the way its phrased here is very ambiguous. it can also be truncated to just "Slice of EBlock entries" with a comment in EBlockBody explaining what the data is

WhoSoup · 2019-10-20T11:13:57Z

common/entryCreditBlock/ecblockHeader.go

@@ -15,37 +15,40 @@ import (
 	"github.com/FactomProject/factomd/common/primitives"
 )

+// ECBlockHeader contains information related to this EC block as well as the previous EC block


"meta information"

WhoSoup · 2019-10-20T11:15:08Z

common/entryCreditBlock/ecblockBody.go

@@ -11,55 +11,64 @@ import (
 	"github.com/FactomProject/factomd/common/primitives"
 )

+// ECBlockBody contains all the entry information for the EC block. The entries will be one of 5 types:
+// CommitChain, CommitEntry, IncreaseBalance, MinuteNumber, or ServerIndexNumber


link to https://github.com/FactomProject/FactomDocs/blob/master/factomDataStructureDetails.md#ecid-bytes

WhoSoup · 2019-10-20T11:16:19Z

common/entryCreditBlock/increasebalance.go

-	Index    uint64                  `json:"index"`
-	NumEC    uint64                  `json:"numec"`
+	ECPubKey *primitives.ByteSlice32 `json:"ecpubkey"` // EC public key that will have balanced increased
+	TXID     interfaces.IHash        `json:"txid"`     // The transaction id associated with this balance increase


i'd shorten that to just "// TXID = transaction id", the rest is redundant

WhoSoup · 2019-10-20T11:17:08Z

common/entryCreditBlock/minutenumber.go

 type MinuteNumber struct {
-	Number uint8 `json:"number"`
+	Number uint8 `json:"number"` // The minute number


WhoSoup · 2019-10-20T11:17:48Z

common/entryCreditBlock/serverindexnumber.go

 type ServerIndexNumber struct {
-	ServerIndexNumber uint8 `json:"serverindexnumber"`
+	ServerIndexNumber uint8 `json:"serverindexnumber"` // the server index number


factablesolutions and others added 6 commits October 17, 2019 20:52

24 files with only comment and receiver name changes

237732e

Modify test so error messages are never out of

04c5a93

sync with their actual values

Comments and receiver changes, two minor code

d648f66

changes. Calling init() and refactor an else statement

Add MaxEntrySizeInBytes const

192d49a

and EC_CHAINID_STRING Comments and receiver changes, and else refactorings

Comments and reciever name changes,

f398d8b

refactor init() call and 'else' statements, delete confusing and mostly duplicative function which is never called outside this file

Merge pull request #900 from factablesolutions/FD-1211_factable_branch

da23912

Fd 1211 factable branch

WhoSoup requested changes Oct 20, 2019

View reviewed changes

stackdump force-pushed the FD-1112_a4_rollup branch 2 times, most recently from ba38a7f to bf73f56 Compare October 29, 2019 18:15

stackdump merged commit 33855bc into FD-1112_a4_rollup Dec 4, 2019

stackdump mentioned this pull request Dec 4, 2019

Comment cleanup after a4 #937

Closed

PaulBernier deleted the FD-1211_factable_solutions_updates_october branch September 26, 2020 04:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fd 1211 factable solutions updates october #901

Fd 1211 factable solutions updates october #901

stackdump commented Oct 18, 2019

WhoSoup left a comment

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

WhoSoup Oct 20, 2019

Fd 1211 factable solutions updates october #901

Fd 1211 factable solutions updates october #901

Conversation

stackdump commented Oct 18, 2019

WhoSoup 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

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

Choose a reason for hiding this comment