-
Notifications
You must be signed in to change notification settings - Fork 71
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
Session key auth #296
Session key auth #296
Conversation
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 code! A couple of changes requested but should be fine!
): | ||
SessionKey_owner_store.write(session_public_key, eth_address) | ||
let (current_timestamp) = get_block_timestamp() | ||
SessionKey_end_timestamp_store.write(session_public_key, current_timestamp + session_duration) |
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.
Should we check for overflow here? What happens if current_timestamp + session_duration
overflows? the number will be either negative, or < current_timestamp
? This means it simply won't work, so might as well throw an error here right?
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 nice spot. could also enforce a max session duration?
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 I think a max duration of a year should be fine to start with, wdyt?
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 just put an overflow check for now, some people might just want an indefinite session key to be active so seems unnecessary to limit it.
with_attr error_message("Overflow in Session duration, use smaller value"):
assert_le(current_timestamp, end_timestamp)
end
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.
Just one question about overflow but otherwise looks good! Very nice work, I want to try it with a UI! :)
Adding 2 session key authenticators, one authorized by an ethereum signature, and one authorized by an ethereum transaction.
closes #254