REST API: Add GetBlockHash API to algod v2#4566
Conversation
Eric-Warehime
requested review from
AlgoStephenAkiki,
cce,
shiqizng and
winder
Codecov Report
@@ Coverage Diff @@
## master #4566 +/- ##
==========================================
- Coverage 54.10% 54.09% -0.02%
==========================================
Files 401 401
Lines 51625 51635 +10
==========================================
Hits 27931 27931
- Misses 21343 21349 +6
- Partials 2351 2355 +4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
winder
changed the title
| err = protocol.DecodeJSON(rec.Body.Bytes(), &blkHashResponse) | ||
| require.NoError(t, err) | ||
| require.Equal(t, crypto.HashObj(blkResponse.Block.BlockHeader).String(), blkHashResponse.BlockHash) | ||
| } |
There was a problem hiding this comment.
Maybe assert that the hash doesn't contain AAAAAAAA (the zero hash)
When I did this, I grabbed two blocks and asserted that block 2's "prev" was the equal to block 1's hash
master...cce:go-algorand:add-v2-block-hash#diff-c9875030556c016ef35a4b47373f5005ee03b961a82157bed9ef734881edddc3R253-R268
There was a problem hiding this comment.
I'm pretty conflicted about the way "prev" is implemented...Because it's wrapped in a BlockHash object which overrides the String() method by prepending "blk-", these would produce different outputs
BlockHash(crypto.Digest([]byte{"AAAAAA"})).String()
crypto.Digest([]byte{"AAAAAA"}).String()
On the other hand, the crypto.Digest String() method does a base-32 encoding of the hash bytes when converting to a string.
I'm tempted to just return bytes from GetBlockHash and let the user do whatever conversion they want, but on the other hand I agree that if a returned BlockHash didn't match up with what was in "prev" it would be very confusing for the end user.
There was a problem hiding this comment.
I'm going to be out for a while so I'll leave the decision up to @winder but in my opinion, returning "blk-1235" as a hash value from this API feels strange.
There was a problem hiding this comment.
Oh good point, which format is "correct" for this purpose, the blk-XXX hash version or just XXX? It seems the REST API is already using the blk- prefixed version for the existing endpoint, so there's one point for consistency. I don't really know much about where else these blk- prefix hashes appear
There was a problem hiding this comment.
My only contributions here are some observations:
- algoexplorer seems to remove the
blk-. - Indexer b64 encodes it.
From this, it seems like everyone agrees that blk- is a little strange so this is a consistency question. For consistency, I think we should select the B32 encoding over B64. If someone wants to compare them they'll have to remove the blk- prefix.
Eric-Warehime
dismissed stale reviews from AlgoStephenAkiki and winder
via
873e31f
|
Moving to #4580 |
4 participants
Summary
Addresses algorand/algorand-sdk-testing#230 by implementing a new endpoint returning simply the hash of a Block header for a given round.
Test Plan
See added tests.