-
Couldn't load subscription status.
- Fork 31
fix(sales): fix marketplace block expiry #1258
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
Conversation
b8eee11 to
6e55e37
Compare
|
Commit |
Previously we used request.expiry as the block expiry, but the former is a duration in seconds, whereas the latter is a timestamp
ddea7c4 to
2036e23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for this!
Was looking where mistake was made and it was mine 😅 So thanks for patching this!
| trace "Retrieving expiry" | ||
| var expiry: SecondsSince1970 | ||
| if state =? requestState and state == RequestState.Started: | ||
| expiry = await market.getRequestEnd(requestId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon the filled state, this will get "re-updated" to the same value, but I guess it is not a problem except a bit of wasted CPU cycles.
| StorageRequested( | ||
| requestId: request.id, | ||
| ask: request.ask, | ||
| expiry: market.requestExpiry[request.id].uint64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the expiry should be the duration no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no sorry, it is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's really confusing keeping these two uint64 values apart, so that's why in #1196 I created different types for durations and timestamps. But I didn't want to extract all of that into this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Previously we used request.expiry as the block expiry, but the former is a duration in seconds, whereas the latter is a timestamp.
Extracted the fix from #1196