exp/clients/horizon: add prev and next methods for Ledger Requests

[poliha]

This PR adds support for the horizonclient to be able to return previous and next result pages for certain endpoints; similar to the JS sdk result.next().
Here, it is implemented for /ledgers. For each page returned, you can call LedgerRequest.Next(returnedPage, client) to get the next set of pages from the Link specified in the returnedPage. This can also the be done to get previous page.

I would like to know your thoughts around having Prev() and Next() on the LedgerRequest struct.
Before settling to implement it in this manner I had some other options

1. Implement Prev() and Next() on the result struct, e.g hProtocol.LedgersPage.Next(). This would mean that I will have to use a new client to fulfil the requests. Using the same horizon client will lead to a cyclic dependency error.

2. Implement Prev() and Next() on the horizon client, e.g Client.Next(resultPage) resultPage. The
   problem with this is that resultPage will have to be of type interface{} in other to be used by all the different result structs.

[bartekn]

From the options you listed I think we should go with number 2. I think we can easily solve passing interface{} by creating a Page interface like:

type Page interface {
   getNextPageURL() (string, error)
   getPrevPageURL() (string, error)
}

Then we just need to implement it on page structs. The example code would be something like:

page, _ := client.Ledgers(LedgerRequest{Order: OrderDesc, Limit: 200})
nextPage, err := client.NextPage(page)

The problem is that users would need to do a type checking (as NextPage returns Page) but it will prevent a lot of code duplication (Next, Prev methods on each page struct).

[bartekn]

Client.sendRequest should really use this method internally.

[bartekn]

Better to use defer cancel() instead of two cancel()s.

[poliha]

Thanks for your suggestion @bartekn the only reservation I have is the type checking for users. But I guess if we balance duplication with usability. If there are no objections, we can implement this. @ire-and-curses @tomquisel ?

[ire-and-curses]

Since this is an SDK, I feel like we should do as much as possible to make it simple for users. I think it's pretty annoying to have to do the type check (user needs to know what type they are expecting, which requires looking at docs).

I'd like to hear if there's any issues with the other idea we discussed internally:

One other option is to move EffectsPage, LedgersPage, OperationsPage etc out of the protocols/horizon package and into the client package. Right now I think it's only the client package that uses those "XPage" structs. Then you should be able to implement Next() on them without running into dependency issues.

If this is viable, then you could implement something that looks more like the JS approach. Having that similarity is valuable IMHO. Overall I don't think creating a bunch of next methods is really too bad. Especially if you could abstract any of it to a helper.

[poliha]

I agree that type checking can be a bit of a drag, but the user really doesn't need to guess because they will be calling next() as a follow up to the initial request and they already know what type that is and you can almost liken type checking the result of next() as a sanity check.

Thanks for bringing up this other idea, I had forgot about this. Yes, I agree this is the closest implementation to the JS SDK. One drawback here is that we might need to use a new http client to fulfil the requests or always pass the current horizon client as an parameter to the Next() method.

[bartekn]

Both solutions are interesting but I think *Page structs should stay in protocols/horizon (and ultimately Horizon should use them as well for rendering). What we could do is create mirror types like:

type EffectsPage struct {
   Page horizonProtocol.EffectsPage
}

func (*p EffectsPage) Next(client *Client) (EffectsPage, error) {
   ...
}

But then devs would need to access page data like: page.Page.

There's also something we can do to prevent using type checks in my initial proposal. We can create Next* methods for each page type, ex.:

page, _ := client.Ledgers(LedgerRequest{Order: OrderDesc, Limit: 200})
nextPage, err := client.NextLedgersPage(page)

Users still need to remember that they're using a specific page type but in case of mistake it would be caught at compilation time.

[poliha]

Hey @bartekn thanks for looking into this a bit more. I like the idea of creating Next* pages because they don't bring about any breaking changes, and its clear from the interface which types support retrieving next and previous pages.
Going to close this and start the implementation based on this idea