Skip to content
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

Basic implementation of liquidity_pools?account #426

Merged

Conversation

vinamogit
Copy link
Contributor

Fixes #376

@@ -26,4 +26,13 @@ public void testForReserves() {
.buildUri();
assertEquals("https://horizon-testnet.stellar.org/liquidity_pools?reserves=EURT%3AGAP5LETOV6YIE62YAM56STDANPRDO7ZFDBGSNHJQIYGGKSMOZAHOOS2S%2CPHP%3AGAP5LETOV6YIE62YAM56STDANPRDO7ZFDBGSNHJQIYGGKSMOZAHOOS2S", uri.toString());
}

@Test
public void testForAccount() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not established here yet on other tests, but might be worthwhile to include coverage of an additional test for clearing the parameter from the builder, i.e.LiquidityPoolsRequestBuilder.forAccount(null) is valid, what would be the expected url if that is called or called in succession after first doing .forAccount("xyz")an allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do that. I tried with forReserves(String...) but of course there is a NPE on forReserves(null).
Does a pass is required on this kind of method call?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forReserves(String ...)interface prevents null parameter, might be worth a bit more coverage to assert a second call with different values for any of these builder methods, and verify if result is supposed to append or reset, It depends on okhttp3.HttpUrl.Builder, then can merge, as the PR looks good.

@sreuland
Copy link
Contributor

sreuland commented May 2, 2022

Looks good to me, thanks for contributing! @tamirms per your #376 , do you agree?

@vinamogit vinamogit force-pushed the feature-liquiditypools-forAccount branch from a582d05 to 225d483 Compare May 3, 2022 20:44
@overcat
Copy link
Member

overcat commented Aug 10, 2023

This PR seems to have been forgotten, I think we can merge it. @sreuland

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Add request for fetching liquidity pools by account
4 participants