-
Notifications
You must be signed in to change notification settings - Fork 30
Unify update recovery logic under an update guard #57
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
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.
Nice. You went to a lot of effort to save users from themselves and also to include tuples 😅
Only had a few comments.
let Some(new_seq) = enr.seq.checked_add(1) else { | ||
revert.recover(enr); | ||
#[cfg(test)] | ||
assert_eq!(&enr_backup, enr); |
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.
We should be careful with these. The Eq implementation doesn't check the content. Just the seq, node-id and signature. We're assuming the sig covers the contents, but in this case, we are just setting it to some value.
I think its fine, just pointing it out
|
||
/* | ||
* Helper traits to expand the definition of Update to tuples and vectors | ||
* NOTE: fixed sixed arrays would need `array::try_map` which is not yet stable |
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.
* NOTE: fixed sixed arrays would need `array::try_map` which is not yet stable | |
* NOTE: fixed sized arrays would need `array::try_map` which is not yet stable |
} | ||
|
||
/// Sets a new public key for the record. | ||
pub fn set_public_key(&mut self, public_key: &K::PublicKey, key: &K) -> Result<(), EnrError> { |
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.
Not sure this is used very much, but is there a reason we explicitly remove this one?
} | ||
|
||
/// Modifies the sequence number of the builder. | ||
pub fn seq(&mut self, seq: u64) -> &mut Self { | ||
self.seq = seq; | ||
self.enr.seq = seq; |
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 there is a subtlety here that may cause trouble.
If I followed correctly, whatever value we set here, when the ENR does the update, the sequence number will increase. So in principle this will be whatever we set + 1.
Also we can no longer create ENR's with 0 sequence numbers.
Not sure if others are doing it or not, but maybe we do want users to set their own sequence numbers or at least modify them if they want to.
If they want to re-use one they have made in the past, the ENR is still valid and they could do it anyway by making a whole new ENR, so I think we prob should support the functionality.
The API I wanted to achieve needs features rust does not have yet. In an ideal world, I'd like to be able to do let (prev_val1, prev_val2, prev_val3) = enr
.update_guard()
.insert(key1, value1)
.insert(key2, value1)
.remove(key3)
.finish()?; for arbitrary number of values this intermediate state I achieved it not good enough to justify the complexity. I'm thus closing |
Update::to_valid
this can be done for tuples of to size up to 7, single updates and vectors.