Add pagination to get_exchange_ids() function #110
Conversation
KendallWeihe
requested review from
nitro-neal,
kirahsapong,
shamilovtim,
diehuxx,
decentralgabe and
frankhinek
as code owners
| let service_endpoint = get_service_endpoint(pfi_did)?; | ||
| let get_exchanges_endpoint = format!("{}/exchanges", service_endpoint); | ||
|
|
||
| let get_exchanges_endpoint = if let Some(params) = query_params { | ||
| add_pagination( |
There was a problem hiding this comment.
we can improve this in due time, but for now this is the most efficient way. namely, we may consider using a dedicated Url type, but which will require additional considerations (namely it may require adding a new dependency which must pass licensing considerations)
There was a problem hiding this comment.
btw we need to add pagination to other functions too, so we can improve DRY and the like at such time
| pub fn get_exchange_ids( | ||
| pfi_did_uri: String, | ||
| bearer_did: Arc<BearerDid>, | ||
| query_params: Option<GetExchangeIdsQueryParams>, |
There was a problem hiding this comment.
if the query_param is absent, does it return the full list?
There was a problem hiding this comment.
that would be logic implemented on the PFI-side, but yes if not included then the expectation would be the full set of exchange IDs
perhaps we should add that language to the spec, or perhaps we should have a maximum as the default and include pagination on the response (@jiyoontbd @mistermoe)
There was a problem hiding this comment.
if there's a million exchange between Alice and PFI already, it might be just a tad bit difficult for PFI to return all that in one response
so i agree with the idea of having a max. we could have it paginated by default showing the last X exchange... i don't really know what X should be. 50? 100?
There was a problem hiding this comment.
this is just a list of ids tho, right, and not the full list of messages (which is an Exchange)?
| pfi_did: &str, | ||
| requestor_did: &BearerDid, | ||
| query_params: Option<GetExchangeIdsQueryParams>, | ||
| ) -> Result<Vec<String>> { | ||
| let service_endpoint = get_service_endpoint(pfi_did)?; |
There was a problem hiding this comment.
What happens with multiple services?
There was a problem hiding this comment.
Good question. The get_service_endpoint is not something exposed internally, so just an internal function for the sake of DRY, and it specifically searched for the service with the type equal to PFI which is the only applicable DID service method in the context of the tbdex::http_client module.
There was a problem hiding this comment.
there could still be multiple 😄
There was a problem hiding this comment.
Very fair point! #108 (comment)
| @@ -628,5 +629,16 @@ CLASS Exchange | |||
| ## `get_exchange_ids()` | |||
|
|
|||
| ```pseudocode! | |||
| FUNCTION get_exchange_ids(pfi_did_uri: string, bearer_did: BearerDid): []string | |||
| FUNCTION get_exchange_ids( | |||
| pfi_did_uri: string, | |||
There was a problem hiding this comment.
Non blocking but is it worth having a DID type?
There was a problem hiding this comment.
Yeah, it's an idea worthy of consideration. I default to primitive types (strings) where possible, because I like to minimize cross-dependency usage (a DID type would originate, and in fact already exists, inside of web5-rs), but that's my bias.
This adds tbdex pagination for the
get_exchange_ids()function from-APID-all-the-way-through-the-example appReference https://github.com/TBD54566975/tbdex/tree/main/specs/http-api#pagination