LiquidityPoolIDDeserializer is missing from the PageDeserializer. #422
Conversation
vinamogit
changed the title
vinamogit
force-pushed
the
fix-lp-id-deserialization-in-page
branch
from
aae1186
to
59d1a06
Compare
| @@ -17,6 +19,16 @@ public void testDeserialize() { | |||
| assertEquals(accountsPage.getLinks().getPrev().getHref(), "/accounts?order=desc&limit=10&cursor=1"); | |||
| assertEquals(accountsPage.getLinks().getSelf().getHref(), "/accounts?order=asc&limit=10&cursor="); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testDeserializeWithLiquidityPoolBalance() { | |||
There was a problem hiding this comment.
great test coverage, do you think also worth adding one more check on regression aspect, i.e.:
assertEquals(accountsPage.getRecords().get(0).getBalances()[1].getLiquidityPoolID(), Optional.absent());
style-wise, good to remove the extra lines.
| @@ -45,6 +46,7 @@ public Page<E> deserialize(JsonElement json, Type typeOfT, JsonDeserializationCo | |||
| .registerTypeAdapter(EffectResponse.class, new EffectDeserializer()) | |||
| .registerTypeAdapter(LiquidityPoolResponse.class, new LiquidityPoolDeserializer()) | |||
| .registerTypeAdapter(TransactionResponse.class, new TransactionDeserializer()) | |||
| .registerTypeAdapter(LiquidityPoolID.class, new LiquidityPoolIDDeserializer()) | |||
There was a problem hiding this comment.
Interesting, I see the same type adapter registration for LiquidityPoolID done up at the main gson instance used by all response handlers:
Do you know why this is not getting used by gson to resolve the LiquidityPoolID in TradeResponse during unmarshaling? It looks TradesRequestBuilder.execute uses the ResponseHandler which in turn uses that gson instance which includes the type registration. If re-registering here at page deser is needed, so be it, nice find!
There was a problem hiding this comment.
I don't know anything about Gson, my guess is that it's either because of generic type or because a new Gsonbuilder is used.
There was a problem hiding this comment.
The thing is that there are several Gson instances, each need to know the deserializers.
We could avoid the double maintenance between GsonSingleton and PageDeserializer by using the JsonAdapter annotation on all the types which are not Page.
The only type that would need to be registered with both GsonBuilders will be the TransactionDeserializer since it uses a third instance of Gson to reconciliate the memo with its memo_type.
I published a branch for comparison:
vinamogit@fe9c9de
vinamogit
force-pushed
the
fix-lp-id-deserialization-in-page
branch
from
59d1a06
to
f3e98d5
Compare
LiquidityPoolIDDeserializer is missing from the PageDeserializer.
This fixes #404 and #421.