-
Notifications
You must be signed in to change notification settings - Fork 60
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
fix: export expiration block number procedures #925
Conversation
63997b4
to
293f9d4
Compare
#! | ||
#! Inputs: [block_height_delta, ...] | ||
#! Output: [...] | ||
#! | ||
#! Where: | ||
#! - block_height_delta is the desired expiration time delta (1 to 0xFFFF). | ||
export.update_expiration_block_num | ||
export.update_expiration_delta |
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 think we want to keep this name the same as it was although I don't mind this option too much either.
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 would keep the name the same here.
update_expiration_delta
procedureThere 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! Left a small comment, let's wait for confirmation before merging.
miden-lib/asm/miden/tx.masm
Outdated
#! | ||
#! Where: | ||
#! - block_height_delta is the desired expiration time delta (1 to 0xFFFF). | ||
export.update_expiration_block_num |
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 think we want update_expiration_block_delta
here (ie, in the user-facing procedure), unless I misunderstood @bobbinth. If this is the case, comment on line 203 should probably be updated as well.
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.
Yes, exactly. From the user's standpoint, we are operating with the delta rather than the block number. So, the user should be setting and getting the current delta. The fact that we are storing it as expiration block number internally, I think, is an implementation details.
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.
Looks good! Thank you! I left a few more comments inline.
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.
All looks good! Thank you!
7b9383f
to
fbfbeb1
Compare
Exposes procedures to get and set the expiration block number so that they can be used by the client