-
Notifications
You must be signed in to change notification settings - Fork 48
feat: adding latest value for thawingUntil on Indexer and Provision #299
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
feat: adding latest value for thawingUntil on Indexer and Provision #299
Conversation
src/mappings/horizonStaking.ts
Outdated
| provision.thawingUntil = event.params.thawingUntil | ||
| provision.save() | ||
|
|
||
| indexer.thawingUntil = event.params.thawingUntil |
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.
This is not necessarily true, that the latest thaw request has the latest thawingUntil, because we could change the thawingPeriod but I think we should not worry about it. Maybe just note it with a comment?
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.
Hmm, I didn't think about it, but that's correct, maybe simply doing a max out of the current value and the next?
andrascodes
left a comment
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.
Thanks! 🙏
|
@tmigone changed the logic so that it's always the max value |
| provision.thawingUntil = event.params.thawingUntil > provision.thawingUntil ? event.params.thawingUntil : provision.thawingUntil | ||
| provision.save() | ||
|
|
||
| indexer.thawingUntil = event.params.thawingUntil > indexer.thawingUntil ? event.params.thawingUntil : indexer.thawingUntil |
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.
Sry I was just reviewing this more closely for the Engineering Plan: don't we have to set indexer.thawingUntil back to 0 in handleThawRequestFulfilled or handleTokensDeprovisioned?
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.
Good catch, I think so. At least on fulfilled it makes sense.
Worst case scenario, the value is easy to identify as invalid though, since if thawingUntil is essentially in the past, it's essentially done thawing
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 actually you're right! shouldn't be a big deal! It's overwritten by a later timestamp anyway and if it's in the past it's withdrawable as you say. nvm then! thx!!!
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.
I'm gonna merge it, but worst case scenario we can modify it later down the road if necessary
ba8bb75
into
juanmardefago/horizon-stage-1-signed
No description provided.