extend explorer api with block.ID() and optional hexblock field by petabytestorage · Pull Request #2418 · NebulousLabs/Sia

petabytestorage · 2017-10-12T16:54:35Z

block.ID() fixes "missing block" explorer issue with blocks with no transaction. "hexblock" optional return field (with "hexblock"="true" optional form parameter) provides verbatim hex encoded binary block to enable external applications and developer scaffolding.

DavidVorick · 2017-10-13T18:39:10Z

api/explorer.go

 		panic("incorrect request to buildExplorerBlock - block does not exist")
 	}

+	hex_block := ""


more idiomatic to do var hex_block string

Project consistently and strictly uses camel case throughout for variable names, need to do the same here.

petabytestorage · 2017-10-13T21:43:16Z

hex_block_enabled -> hexBlockEnabled and hex_block -> hexBlock ty @DavidVorick

DavidVorick · 2017-10-14T10:27:53Z

api/explorer.go

 	WriteJSON(w, ExplorerBlockGET{
-		Block: api.buildExplorerBlock(height, block),
+		Block: api.buildExplorerBlock(height, block,
+			req.FormValue("hexblock") == "true"),


Sorry I didn't comment on this in the first pass. Wasn't sure what we wanted here, but after discussing with Luke, we agree that the call should return an error if hexblock is set to anything besides "true" or "false" - strict api reduces the liklihood of dev mistakes, and coherent errors around incorrect input make it easier to find the source of the mistake.

Makes sense to me and I will do it. I copied this pattern from api/wallet.go where it appears twice and does only look for "true". I also guessed that the wallet.go would be more strict/secure than an explorer module. So I'm going to suggest the wallet.go "force" field should be considered as well if you want to use this pattern for example to avoid misleading other devs into copying around or learning a bad pattern. @DavidVorick

Can you give me an example of the new pattern you and luke want in the pre-existing source code please? I'm looking through all the boolean flags in the system and not finding one "good" one according to your description.

as per our discord discussion, we'll punt this to a follow-up PR

DavidVorick · 2017-10-14T10:28:01Z

api/explorer.go

 			HashType: "blockid",
-			Block:    api.buildExplorerBlock(height, block),
+			Block: api.buildExplorerBlock(height, block,
+				req.FormValue("hexblock") == "true"),


DavidVorick · 2017-10-14T10:28:40Z

LGTM except for the final comments. Looking for review from @lukechampine

lukechampine · 2017-10-14T15:31:37Z

api/explorer.go


+	var hexBlock string
+	if hexBlockEnable {
+		hexBlock = fmt.Sprintf("%x", encoding.Marshal(block)[:])


encoding.Marshal returns a []byte so the [:] slicing is unnecessary

also, I think we slightly prefer:

import "encoding/hex" hex.EncodeToString(encoding.Marshal(block))

lukechampine · 2017-10-14T15:31:41Z

api/explorer.go

 		Transactions:   etxns,
 		RawBlock:       block,
+		HexBlock:       hexBlock,
+		BlockId:        fmt.Sprintf("%x", encoding.Marshal(block.ID())[:]),


lukechampine · 2017-10-15T18:09:30Z

api/explorer.go

 		Transactions   []ExplorerTransaction   `json:"transactions"`
 		RawBlock       types.Block             `json:"rawblock"`
+		HexBlock       string                  `json:"hexblock"`
+		BlockId        string                  `json:"blockid"`


should be BlockID

lukechampine · 2017-10-15T18:16:40Z

api/explorer.go

+	var hexBlock string
+	if hexBlockEnable {
+		hexBlock = hex.EncodeToString(encoding.Marshal(block))
+	}


for structs with optional fields, I generally prefer:

b := ExplorerBlock{ // required fields here... } if hexBlockEnable { b.HexBlock = hex.EncodeToString(encoding.Marshal(block)) } return b

petabytestorage · 2017-10-15T20:57:20Z

all done @DavidVorick and @lukechampine thanks for the idiomatic go

lukechampine · 2017-10-16T06:59:33Z

api/explorer.go

+		HexBlock       string                  `json:"hexblock"`
+		BlockID        string                  `json:"blockid"`

 		modules.BlockFacts


hmm, changing to BlockID actually caused tests to start failing, because modules.BlockFacts has an embedded BlockID field.

Is it necessary to have a BlockID string field if we already have it as a [32]byte? If so, maybe call it HexBlockID?

@lukechampine when i remove the field i get:

➜ Sia git:(explorer-patch) ✗ make go install -tags 'netgo' -ldflags='-s -w' ./api ./build ./compatibility ./crypto ./encoding ./modules ./modules/consensus ./modules/explorer ./modules/gateway ./modules/host ./modules/host/contractmanager ./modules/renter ./modules/renter/contractor ./modules/renter/hostdb ./modules/renter/hostdb/hosttree ./modules/renter/proto ./modules/miner ./modules/wallet ./modules/transactionpool ./persist ./siac ./siad ./sync ./types # github.com/NebulousLabs/Sia/api api/explorer.go:201: unknown field 'BlockID' in struct literal of type ExplorerBlock Makefile:75: recipe for target 'release-std' failed make: *** [release-std] Error 2

therefore i renamed it to HexBlockID however i left the json named the standard way because to me it seems cleaner. please feel free to adjust again if you think there is a cleaner way to handle this.

ChrisSchinnerl

Looks pretty good to me. I'd only like to see a few minor changes that centralize the encoding of the types a bit more.

ChrisSchinnerl · 2018-03-05T18:35:06Z

api/explorer.go

@@ -188,13 +194,18 @@ func (api *API) buildExplorerBlock(height types.BlockHeight, block types.Block)
 		panic("incorrect request to buildExplorerBlock - block does not exist")


In my opinion we shouldn't crash if a user supplies an invalid height but return an error we write to the http response. What do you think @lukechampine @DavidVorick

ChrisSchinnerl · 2018-03-05T18:48:08Z

api/explorer.go

 		Transactions   []ExplorerTransaction   `json:"transactions"`
 		RawBlock       types.Block             `json:"rawblock"`
+		HexBlock       string                  `json:"hexblock"`
+		HexBlockID     string                  `json:"blockid"`


HexBlockID can probably be BlockID and have types.BlockID as a type.
types.BlockID implements String and MarshalJSON which should implicitly convert it to a hex string in WriteJSON.

ChrisSchinnerl · 2018-03-05T18:49:24Z

api/explorer.go

 		MinerPayoutIDs: mpoids,
 		Transactions:   etxns,
 		RawBlock:       block,
+		HexBlockID:     hex.EncodeToString(encoding.Marshal(block.ID())),


As mentioned above this can probably just be a type.BlockID instead of having to encode yourself. That means it would be BlockID: block.ID(),

ChrisSchinnerl · 2018-03-05T18:52:47Z

api/explorer.go

 		BlockFacts: facts,
 	}
+	if hexBlockEnable {
+		b.HexBlock = hex.EncodeToString(encoding.Marshal(block))


Could you implement something like the following in types/block.go?

// String returns a hex encoded marshalled Block. func (b Block) String() string { return hex.EncodeToString(encoding.Marshal(b)) }

That way you can simply do b.HexBlock = block.String() here.

extend explorer api with block.ID() and optional hexblock field

d8e7ae0

petabytestorage force-pushed the explorer-patch branch from 0bcd834 to d8e7ae0 Compare October 12, 2017 16:55

DavidVorick reviewed Oct 13, 2017

View reviewed changes

fix variable names to camelCase

7b6e39c

switch to 'var hexBlock string' convention

8384a40

DavidVorick reviewed Oct 14, 2017

View reviewed changes

lukechampine reviewed Oct 14, 2017

View reviewed changes

do luke comments

0b81134

lukechampine reviewed Oct 15, 2017

View reviewed changes

get rid of extra variable

88a7a5c

petabytestorage force-pushed the explorer-patch branch from 7414741 to 88a7a5c Compare October 15, 2017 20:56

lukechampine reviewed Oct 16, 2017

View reviewed changes

rename to HexBlockID

954c25b

ChrisSchinnerl suggested changes Mar 5, 2018

View reviewed changes

		@@ -188,13 +194,18 @@ func (api *API) buildExplorerBlock(height types.BlockHeight, block types.Block)
		panic("incorrect request to buildExplorerBlock - block does not exist")

Conversation

petabytestorage commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petabytestorage commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidVorick commented Oct 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petabytestorage commented Oct 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisSchinnerl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants