refactor historical API #1930
refactor historical API #1930
Conversation
| // the following should not happen, as we maintain only blockWindow entries; however, | ||
| // if it does, the following would correct that. |
There was a problem hiding this comment.
Since the block window can change on restart, this comment is not accurate ie. we may hit this when the block window shortens such that we should remove blocks
| // ensure that lastHeight always contains the highest height we've seen. | ||
| i.lastHeight = max(i.lastHeight, blk.Block.Hght) |
There was a problem hiding this comment.
Why do we use this here? We should only ever receive blocks in order.
| return nil | ||
| } | ||
|
|
||
| func (i *Indexer) Notify(_ context.Context, blk *chain.ExecutedBlock) error { | ||
| i.mu.Lock() |
There was a problem hiding this comment.
Do we really need to grab the lock while writing to the database?
I believe using an atomic should be sufficient and removes the need to hold the lock during database operations.
We only read the last height during GetLatestBlock, if it's updated during that time, it should be fine to return a slightly outdated latest block. If a client performs sequential calls, this should still provide them a monotonically increasing view of the latest block.
| return nil | ||
| } | ||
|
|
||
| func (i *Indexer) Notify(_ context.Context, blk *chain.ExecutedBlock) error { | ||
| i.mu.Lock() | ||
| defer i.mu.Unlock() |
There was a problem hiding this comment.
let's add a blank line after grab and defer releasing the lock for consistent style
| i.mu.RLock() | ||
| defer i.mu.RUnlock() | ||
| return i.GetBlockByHeight(i.lastHeight) |
There was a problem hiding this comment.
Same comment here for empty line after deferring the lock
| @@ -55,4 +63,5 @@ func (f *FIFO[K, V]) Get(key K) (V, bool) { | |||
| // [WriteLock] is held when this is accessed. | |||
| func (f *FIFO[K, V]) remove(key K) { | |||
| delete(f.m, key) | |||
| f.removedKey = &key | |||
There was a problem hiding this comment.
Hmm it seems fairly odd to pass around this data via an internal pointer modified via the callback in the bounded buffer.
It seems that we added this to return the removed key for the indexer, but I don't think we should modify the fifo cache with this hack to support that specific case. Is there a reason not to use DeleteRange to make sure that we've removed everything below our desired height?
| type GetBlockByHeightRequest struct { | ||
| Height uint64 `json:"height"` | ||
| BlockID ids.ID `json:"blockID"` | ||
| BlockNumber uint64 `json:"blockNumber"` |
There was a problem hiding this comment.
(out of scope for this PR) Can we add an Encoding argument that can be either json or hex, defaults to hex, and specifies which field in the response to fill out ie. we populate either the block bytes or block field, but not both?
Can we apply the same to the transaction and either populate the JSON of the transaction or the transaction bytes?
| @@ -53,8 +56,8 @@ func (c *Client) GetBlockByHeight(ctx context.Context, height uint64, parser cha | |||
| resp := GetBlockClientResponse{} | |||
| err := c.requester.SendRequest( | |||
| ctx, | |||
| "getBlockByHeight", | |||
| &GetBlockByHeightRequest{Height: height}, | |||
| "getBlock", | |||
There was a problem hiding this comment.
Out of scope for this PR - we should move the parser from an argument to each function to an argument to construct the client instance, so that we don't need to pass it around each time
| if found && response.Result != nil { | ||
| success = response.Result.Success | ||
| fee = response.Result.Fee | ||
| } |
There was a problem hiding this comment.
Do we need the check for result to be nil? We should guarantee this is never the case
| f.removedKey = nil | ||
| f.buffer.Push(key) // Push removes the oldest [K] if we are at the [limit] | ||
| removedKey = f.removedKey | ||
| f.removedKey = nil |
There was a problem hiding this comment.
Why do we add f.removedKey = nil both before and after? It seems we should only ever need one.
What ?
This PR is the first refactor ovehaul for the hyper sdk indexer.
The PR addresses consistency and interface functionality.
Changes
GetBlockByHeight,GetBlockByHash,GetLatestBlock) intoGetBlock. The structure parameter forGetBlockwould provide the same functionality previously provided by the individual endpoints.Issue
#1923