feat: add cursor-based pagination options #72
Conversation
There was a problem hiding this comment.
Looks solid to me 💯
Waiting on a review by @ajnavarro before moving forward 🙏
| fromKey := keyBlock(fromBlockNum) | ||
|
|
||
| if toBlockNum == 0 { | ||
| toBlockNum = math.MaxInt64 | ||
| } else { |
There was a problem hiding this comment.
What if the toBlockNum is math.MaxUint64?
There was a problem hiding this comment.
Good point!
I'll add exception handling for large numbers.
| @@ -222,17 +225,18 @@ type PebbleBlockIter struct { | |||
| i *pebble.Iterator | |||
| s *pebble.Snapshot | |||
|
|
|||
| init bool | |||
| init bool | |||
| ascending bool | |||
There was a problem hiding this comment.
This should be true by default, to keep with current expected functionality
There was a problem hiding this comment.
This PR needs several changes:
We should create a PebbleReverseTxIterator that will be used in a new storage method called ReverseTxIterator to avoid all the custom logic used with that reverse boolean all around the code.
Cursor type has highly coupled code that is related to several use cases.
My proposal is to make Cursor an interface with several implementations:
type Cursor interface {
json.Marshaller
ID() (string, error)
}
type BlockHeightCursor uint64
// TODO implement marshaller
func (bhc BlockHeightCursor) ID() (string, error) {
// TODO: implement
return "", nil
}
type BlockHeightTxIndexCursor struct {
Height uint64
TxIndex uint32
}
// TODO implement marshaller
func (bhtc BlockHeightTxIndexCursor) ID() (string, error) {
// TODO: implement
return "", nil
}More changes might be needed after these ones, I didn't do a deep review of the pagination logic.
|
|
||
| // Create a cursor by base64-encoding the ID. | ||
| func NewCursor(id string) Cursor { | ||
| return Cursor(base64.StdEncoding.EncodeToString([]byte(id))) |
There was a problem hiding this comment.
why do we need to encode the id string to base64?
| switch input := input.(type) { | ||
| case string: | ||
| *c = Cursor(input) | ||
| case int32: |
There was a problem hiding this comment.
what if a cursor is an int? this code is a bit flaky, making it difficult to keep it compatible with new types, like int.
| // MarshalJSON encodes a cursor to JSON for transport. | ||
| func (c Cursor) MarshalJSON() ([]byte, error) { | ||
| return strconv.AppendQuote(nil, string(c)), nil | ||
| } |
There was a problem hiding this comment.
why do we need a custom marshaller? Cursor is already a string and it will be serialized as intended.
| // Get the block height from the ID of the Cursor. | ||
| func (c *Cursor) BlockHeight() (uint64, error) { | ||
| id, err := c.ID() | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| if id == "" { | ||
| return 0, nil | ||
| } | ||
|
|
||
| blockHeight, err := strconv.ParseUint(id, 0, 64) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| return blockHeight, nil | ||
| } |
There was a problem hiding this comment.
this is highly coupled code. We are trying to do two totally different behaviors on the same Cursor type...
| // Adjusts the iterator range based on the value of after cursor. | ||
| if afterBlockHeight > 0 { | ||
| if ascending { | ||
| if fromBlockHeight <= afterBlockHeight { | ||
| fromBlockHeight = afterBlockHeight + 1 | ||
| fromIndex = uint32(afterIndex) | ||
| } | ||
| } else { | ||
| if toBlockHeight == 0 || toBlockHeight >= afterBlockHeight { | ||
| toBlockHeight = afterBlockHeight - 1 | ||
| toIndex = uint32(afterIndex) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
this doesn't belong here, that logic should be on the iterator.
| @@ -172,17 +174,16 @@ func (s *Pebble) BlockIterator(fromBlockNum, toBlockNum uint64) (Iterator[*types | |||
| return nil, multierr.Append(snap.Close(), err) | |||
| } | |||
|
|
|||
| return &PebbleBlockIter{i: it, s: snap}, nil | |||
| return &PebbleBlockIter{i: it, s: snap, ascending: ascending}, nil | |||
| } | |||
|
|
|||
| func (s *Pebble) TxIterator( | |||
There was a problem hiding this comment.
instead of sending a flag for reversing the iterator, it might be less convoluted to create a method for the functionality you want to have, in that case, ReverseTxIterator
Doing that you don't need to tweak ranges outside the iterator logic itself.
|
Hey @jinoosss, is this PR still relevant? 🙏 |
|
@zivkovicmilos @ajnavarro , I think this PR is still useful. I'll take a look at your change request by the end of the week 🙏 Additionally, what do you think about adding this functionality to the |
|
Hey @jinoosss ! Thanks for your reply.
Yes, I'm figuring out how to add pagination for the new filters. I'll open a PR ASAP. |
|
@ajnavarro @jinoosss what's the status of this PR? |
|
@zivkovicmilos, I'm waiting for the requested changes to be applied from my side! |
Descriptions
Add a query option to utilize the indexer.
Changes in this PR.
Added
after,size,ascendingoptions totransactions,blocksquery input parameters.after: Retrieves the next value at the after cursor.size: The size of the list being retrieved.ascending: Sort in ascending or descending order. (default: true)Added
pageInfoandedgesdata to response.pageInfo: PageInfo is an information about the current page of edges.edges: Edges contains provided edges of the sequential list. The edge data can retrieve cursor information along with data such as block, transaction, etc.Example