Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d8e7ae0
extend explorer api with block.ID() and optional hexblock field
petabytestorage Oct 12, 2017
7b6e39c
fix variable names to camelCase
petabytestorage Oct 13, 2017
8384a40
switch to 'var hexBlock string' convention
petabytestorage Oct 13, 2017
0b81134
do luke comments
petabytestorage Oct 15, 2017
88a7a5c
get rid of extra variable
petabytestorage Oct 15, 2017
954c25b
rename to HexBlockID
petabytestorage Oct 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions api/explorer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package api

import (
"encoding/hex"
"fmt"
"net/http"

"github.com/NebulousLabs/Sia/build"
"github.com/NebulousLabs/Sia/crypto"
"github.com/NebulousLabs/Sia/encoding"
"github.com/NebulousLabs/Sia/modules"
"github.com/NebulousLabs/Sia/types"

Expand All @@ -20,6 +22,8 @@ type (
MinerPayoutIDs []types.SiacoinOutputID `json:"minerpayoutids"`
Transactions []ExplorerTransaction `json:"transactions"`
RawBlock types.Block `json:"rawblock"`
HexBlock string `json:"hexblock"`
BlockID string `json:"blockid"`

modules.BlockFacts
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

}
Expand Down Expand Up @@ -171,8 +175,10 @@ func (api *API) buildExplorerTransaction(height types.BlockHeight, parent types.
}

// buildExplorerBlock takes a block and its height and uses it to construct an
// explorer block.
func (api *API) buildExplorerBlock(height types.BlockHeight, block types.Block) ExplorerBlock {
// explorer block. hexBlockEnable is a required flag and indicates if the
// hexBlock field should be empty string (false) or
// should contain the hexadecimal binary encoded block (true).
func (api *API) buildExplorerBlock(height types.BlockHeight, block types.Block, hexBlockEnable bool) ExplorerBlock {
var mpoids []types.SiacoinOutputID
for i := range block.MinerPayouts {
mpoids = append(mpoids, block.MinerPayoutID(uint64(i)))
Expand All @@ -188,13 +194,18 @@ func (api *API) buildExplorerBlock(height types.BlockHeight, block types.Block)
panic("incorrect request to buildExplorerBlock - block does not exist")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

return ExplorerBlock{
b := ExplorerBlock{
MinerPayoutIDs: mpoids,
Transactions: etxns,
RawBlock: block,
BlockID: hex.EncodeToString(encoding.Marshal(block.ID())),

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

Choose a reason for hiding this comment

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

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.

}
return b
}

// explorerHandler handles API calls to /explorer/blocks/:height.
Expand All @@ -214,7 +225,8 @@ func (api *API) explorerBlocksHandler(w http.ResponseWriter, req *http.Request,
return
}
WriteJSON(w, ExplorerBlockGET{
Block: api.buildExplorerBlock(height, block),
Block: api.buildExplorerBlock(height, block,
req.FormValue("hexblock") == "true"),
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

})
}

Expand All @@ -231,7 +243,7 @@ func (api *API) buildTransactionSet(txids []types.TransactionID) (txns []Explore

// Check if the block is the transaction.
if types.TransactionID(block.ID()) == txid {
blocks = append(blocks, api.buildExplorerBlock(height, block))
blocks = append(blocks, api.buildExplorerBlock(height, block, false))
} else {
// Find the transaction within the block with the correct id.
for _, t := range block.Transactions {
Expand Down Expand Up @@ -271,7 +283,8 @@ func (api *API) explorerHashHandler(w http.ResponseWriter, req *http.Request, ps
if exists {
WriteJSON(w, ExplorerHashGET{
HashType: "blockid",
Block: api.buildExplorerBlock(height, block),
Block: api.buildExplorerBlock(height, block,
req.FormValue("hexblock") == "true"),
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

})
return
}
Expand Down