-
Notifications
You must be signed in to change notification settings - Fork 504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
clients/horizonclient: Add support for Accounts endpoint. #2229
clients/horizonclient: Add support for Accounts endpoint. #2229
Conversation
Extend the horizonclient to support accounts filter. It allows you to retrive all the accounts with a given signer or with a trustline to an asset. ```go client := horizonclient.DefaultPublicNetClient accountsRequest := horizonclient.AccountsRequest{Signer: "GCLWGQPMKXQSPF776IU33AH4PZNOOWNAWGGKVTBQMIC5IMKUNP3E6NVU"} account, err := client.Accounts(accountsRequest) if err != nil { fmt.Println(err) return } fmt.Print(account) ```
1cca0b0
to
f9a0229
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it does the right thing, just a couple of comments
Limit uint | ||
} | ||
|
||
// AccountRequest struct contains data for making requests to the show account endpoint of a horizon server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem great that we have two structs with very similar names AccountsRequest
vs AccountRequest
. Is there something we can do to name them more clearly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't really like the plural name AccountsRequest`)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ire-and-curses I agree with you, initially I thought about following a similar approach for EffectRequest
, but it seems to me that we are trying to do too much with those structs.
That said though, we could be consistent and try to follow a similar approach. Basically extend the AccountRequest
struct to also take ForSigner
and ForAsset
. When called through client.Accounts
they will be the only fields we look at + Page params, ignoring the rest.
I personally don't like that interface, but it is consistent with how the other *Request
like structs behave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is okay given that we can't make it more consistent with other functions without it being backwards compatible, and the alternatives of smooshing the two types together into a single shared types would be worse and could result in bad human errors. The compiler will make sure the caller uses the right type so whilst there might be some confusion with similar named types any errors arising from that should be prevented.
Co-Authored-By: Eric Saunders <ire-and-curses@users.noreply.github.com>
* clients/horizonclient: Add support for Accounts endpoint. Extend the horizonclient to support accounts filter. It allows you to retrive all the accounts with a given signer or with a trustline to an asset. ```go client := horizonclient.DefaultPublicNetClient accountsRequest := horizonclient.AccountsRequest{Signer: "GCLWGQPMKXQSPF776IU33AH4PZNOOWNAWGGKVTBQMIC5IMKUNP3E6NVU"} account, err := client.Accounts(accountsRequest) if err != nil { fmt.Println(err) return } fmt.Print(account) ``` * Apply suggestions from code review Co-Authored-By: Eric Saunders <ire-and-curses@users.noreply.github.com> * Remove if. * Typos. Co-authored-by: Eric Saunders <ire-and-curses@users.noreply.github.com>
* clients/horizonclient: Add support for Accounts endpoint. Extend the horizonclient to support accounts filter. It allows you to retrive all the accounts with a given signer or with a trustline to an asset. ```go client := horizonclient.DefaultPublicNetClient accountsRequest := horizonclient.AccountsRequest{Signer: "GCLWGQPMKXQSPF776IU33AH4PZNOOWNAWGGKVTBQMIC5IMKUNP3E6NVU"} account, err := client.Accounts(accountsRequest) if err != nil { fmt.Println(err) return } fmt.Print(account) ``` * Apply suggestions from code review Co-Authored-By: Eric Saunders <ire-and-curses@users.noreply.github.com> * Remove if. * Typos. Co-authored-by: Eric Saunders <ire-and-curses@users.noreply.github.com>
* clients/horizonclient: Add support for Accounts endpoint. Extend the horizonclient to support accounts filter. It allows you to retrive all the accounts with a given signer or with a trustline to an asset. ```go client := horizonclient.DefaultPublicNetClient accountsRequest := horizonclient.AccountsRequest{Signer: "GCLWGQPMKXQSPF776IU33AH4PZNOOWNAWGGKVTBQMIC5IMKUNP3E6NVU"} account, err := client.Accounts(accountsRequest) if err != nil { fmt.Println(err) return } fmt.Print(account) ``` * Apply suggestions from code review Co-Authored-By: Eric Saunders <ire-and-curses@users.noreply.github.com> * Remove if. * Typos. Co-authored-by: Eric Saunders <ire-and-curses@users.noreply.github.com>
* clients/horizonclient: Add support for Accounts endpoint. Extend the horizonclient to support accounts filter. It allows you to retrive all the accounts with a given signer or with a trustline to an asset. ```go client := horizonclient.DefaultPublicNetClient accountsRequest := horizonclient.AccountsRequest{Signer: "GCLWGQPMKXQSPF776IU33AH4PZNOOWNAWGGKVTBQMIC5IMKUNP3E6NVU"} account, err := client.Accounts(accountsRequest) if err != nil { fmt.Println(err) return } fmt.Print(account) ``` * Apply suggestions from code review Co-Authored-By: Eric Saunders <ire-and-curses@users.noreply.github.com> * Remove if. * Typos. Co-authored-by: Eric Saunders <ire-and-curses@users.noreply.github.com>
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Extend the horizonclient to support accounts filtering. It allows account retrieval by signer or with a trustline to an asset.
Why
This is part of the work in #2014, which is required to have full compatibility with Horizon 1.0.
Known limitations
[TODO or N/A]