Feat/get all live opps #205
Conversation
| .take(repository::OPPORTUNITY_PAGE_SIZE as usize) | ||
| .take(std::cmp::min( | ||
| query_params.limit, | ||
| OPPORTUNITY_PAGE_SIZE_CAP as usize, |
There was a problem hiding this comment.
a better place to handle this is at the beginning of the function. we shouldn't implicitly cap it but return an error if it's larger than this cap
There was a problem hiding this comment.
i think it's better to return the max rather than error out. maybe we could add a note/warning field to the message, but i think it's bad to error here since the user can still recover some data and go on to recover more data by paginating
There was a problem hiding this comment.
people are not going to read the docs carefully and will use a large number here. Let's say a thousand. Then they will see that the endpoint is returning them 44 values and think ok, this would work later on and can return 1000 values. This would lead to a silent failure and unnoticed bugs but if we validate everything in the beginning and throw errors they will catch it in testing phase and wouldn't have any issues on prod.
| @@ -163,7 +163,7 @@ impl TryFrom<repository::Opportunity<repository::OpportunityMetadataEvm>> for Op | |||
| Ok(OpportunityEvm { | |||
| core_fields: OpportunityCoreFields { | |||
| id: val.id, | |||
| creation_time: val.creation_time.assume_utc().unix_timestamp_nanos(), | |||
| creation_time: val.creation_time.assume_utc().unix_timestamp_nanos() / 1000, | |||
There was a problem hiding this comment.
good catch, I wonder how this was working correctly so far 🤔
sdk/js/src/index.ts
Outdated
| const client = createClient<paths>(this.clientOptions); | ||
| const fromTimeStr = fromTime | ||
| ? `${new Date(fromTime / 1000).toISOString().replace("Z", "")}${( |
There was a problem hiding this comment.
you don't need to convert to microsecond format. I suggest getting the fromTime param as a Date type similar to what you did in the python sdk
There was a problem hiding this comment.
that's also an option. i did it this way to make it easier for the user to specify a microsecond time and have it query that, since Date() does not keep the microsecond-level granularity
There was a problem hiding this comment.
i could also make fromTime a Date and leave it to the user to specify that. i think i slightly prefer handling it for the user so as to mitigate their chances of unintended behavior with Date, but i'm not wedded to this. wdyt?
There was a problem hiding this comment.
I don't think anybody is going to use such precise time formats anyway. If it's easier for user to use a Date object and do some simple day arithmetics with it, I would prefer accepting a Date object here.
| @@ -21,7 +21,7 @@ mod remove_opportunities; | |||
| mod remove_opportunity; | |||
|
|
|||
| pub use models::*; | |||
| pub const OPPORTUNITY_PAGE_SIZE: i32 = 20; | |||
| pub const OPPORTUNITY_PAGE_SIZE_CAP: i32 = 100; | |||
| @@ -678,7 +685,7 @@ pub async fn post_opportunity( | |||
|
|
|||
| /// Fetch opportunities ready for execution or historical opportunities | |||
| /// depending on the mode. You need to provide `chain_id` for historical mode. | |||
| /// Opportunities are sorted by creation time in ascending order in historical mode. | |||
| /// Opportunities are sorted by creation time in ascending order. | |||
| /// Total number of opportunities returned is limited by 20. | |||
| .take(repository::OPPORTUNITY_PAGE_SIZE as usize) | ||
| .take(std::cmp::min( | ||
| query_params.limit, | ||
| OPPORTUNITY_PAGE_SIZE_CAP as usize, |
There was a problem hiding this comment.
people are not going to read the docs carefully and will use a large number here. Let's say a thousand. Then they will see that the endpoint is returning them 44 values and think ok, this would work later on and can return 1000 values. This would lead to a silent failure and unnoticed bugs but if we validate everything in the beginning and throw errors they will catch it in testing phase and wouldn't have any issues on prod.
sdk/js/src/index.ts
Outdated
| const client = createClient<paths>(this.clientOptions); | ||
| const fromTimeStr = fromTime | ||
| ? `${new Date(fromTime / 1000).toISOString().replace("Z", "")}${( |
There was a problem hiding this comment.
I don't think anybody is going to use such precise time formats anyway. If it's easier for user to use a Date object and do some simple day arithmetics with it, I would prefer accepting a Date object here.
| #[param(example="2024-05-23T21:26:57.329954Z", value_type = Option<String>)] | ||
| #[serde(default, with = "crate::serde::nullable_datetime")] | ||
| pub from_time: Option<OffsetDateTime>, | ||
| /// The maximum number of opportunities to return. Capped at 100. | ||
| #[param(example = "20", value_type = usize)] |
There was a problem hiding this comment.
I think a better way to include the 100 parameter is using the maximum attribute in the param.
Even better we can use something like this to automate the validation:
https://github.com/Keats/validator
This PR adds options to help users get all live opportunities from the server, by paginating by time and altering the number of returned opportunities in the response.