feat(nexus): query to retrieve transfers #1221
Conversation
jcs47
requested review from
cgorenflo,
haiyizxx and
fish-sammy
as code owners
sdaveas
force-pushed
the
nexus_query_transfers
branch
2 times, most recently
from
2b5f74b
to
d074559
Compare
proto/nexus/v1beta1/query.proto
Outdated
|
|
||
| message TransfersForChainResponse { | ||
| repeated Transfer transfers = 1 [ (gogoproto.nullable) = false ]; | ||
| int32 count = 2; |
There was a problem hiding this comment.
do we really need an extra field @jack0son? You already get the count implicitly with the array
| // transfers for the specified chain | ||
| message TransfersForChainRequest { | ||
| string chain = 1; | ||
| exported.v1beta1.TransferState state = 2; |
There was a problem hiding this comment.
The reason why I originally used a string instead of the actual type was because of the rest RPC endpoint. If we go about it like this, we will need to pass an integer value as a parameter instead of a string. I don't think that is very user friendly.
There was a problem hiding this comment.
we don't if we don't translate the request into path parameters (which doesn't make sense with the pagination anymore anyway)
| } | ||
|
|
||
| message TransfersForChainResponse { | ||
| repeated exported.v1beta1.CrossChainTransfer transfers = 1 |
There was a problem hiding this comment.
I also originally used a custom protobuf to return the transfers because most of the fields of the exported.v1beta1.CrossChainTransfer type would look weird and some would be redundant, like the Transfer Type field. Unless the microservice does need the full exported.v1beta1.CrossChainTransfer type?
There was a problem hiding this comment.
the response values were literally the same values, just as strings as in CrossChainTransfer
There was a problem hiding this comment.
Not quite, because the response now will contain the entire nexus.Chain struct. I am not sure we need that much information.
There was a problem hiding this comment.
ok fair point, I didn't realize it was such a big structure
There was a problem hiding this comment.
@jcs47 I don't think for now the chain object is a big issue and we need this in, so if you feel strongly about it, please open another issue/PR to create a leaner response struct 🙂
Description
Todos
Steps to Test
Expected Behaviour
Other Notes