Skip to content
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: add approximate_expiration_time to oracles #1390

Merged
merged 3 commits into from
Jun 13, 2023
Merged

Conversation

sborrazas
Copy link
Contributor

refs #1371

@sborrazas sborrazas requested a review from jyeshe June 12, 2023 14:46
@sborrazas sborrazas self-assigned this Jun 12, 2023
Copy link
Member

@jyeshe jyeshe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for me and estimates well. Only some refactoring opportunities.

Comment on lines +57 to +62
def last_gen_and_time(state) do
case State.prev(state, Model.Block, nil) do
{:ok, {height, _mbi}} ->
Model.block(hash: block_hash) = State.fetch!(state, Model.Block, {height, -1})

{height, block_time(block_hash)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to leave the block_time to be used only by the height_to_time? This way the block read would happen once and the conversion from gen to time is left only for the moment it is used (with the additional benefit of one less parameter and skipping the need for last_gen_and_time).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would cause to retrieve the time for the last block multiple times when requesting it for a list of oracles: https://github.com/aeternity/ae_mdw/pull/1390/files#diff-d800a239402e86baa72793fe422de5f1d247ad3452b2efd3ee981cede91c038eR298

@@ -332,6 +333,8 @@ defmodule AeMdw.Oracles do
active: is_active?,
active_from: register_height,
expire_height: expire_height,
approximate_expiration_time:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what you think of estimated_expire_time? it sounds to give a closer idea of the time it will expire and not the duration until it expires, following the style of expire_height .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on leaving expire instead of expiration to follow expire_height format. Left the approximate because it works well for both past and future dates

@sborrazas sborrazas requested a review from jyeshe June 13, 2023 10:53
@sborrazas sborrazas merged commit b008217 into master Jun 13, 2023
7 checks passed
@sborrazas sborrazas deleted the oracles-exp-time branch June 13, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants