-
Notifications
You must be signed in to change notification settings - Fork 379
Make functionality to read relay state proof entries public #1135
Conversation
@@ -273,4 +277,18 @@ impl RelayChainStateProof { | |||
) | |||
.map_err(Error::UpgradeRestriction) | |||
} | |||
|
|||
pub fn read_entry<T>(&self, key: &[u8], fallback: Option<T>) -> Result<T, Error> |
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.
Why not directly expose the functions to read the babe randomness? As we do for all the other values?
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.
Then we will need functions for every value we would like to get. This will be at least:
read_current_block_randomness
read_one_epoch_ago_randomness
read_two_epochs_ago_randomness
read_epoch_index
It is less code to write one read_entry<T>
which is generic over the successful result value returned.
Why not directly expose the functions to read the babe randomness? As we do for all the other values?
This approach also requires adding a new function if we ever want to read a new value from the RelayChainStateProof
and that comes with a release delay so it is not preferred.
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 get what you say, but currently it also requires that you add these values before on the client side :P
We can add this function, I don't really care, but then please add some docs :)
@bkchr Any suggestions to get the CI green? It succeeded on the only commit which changed the code, but is now failing after added doc comments.
|
@4meta5 can you please merge master |
Follow up for #1083 which makes the functionality to read arbitrary entries in the relay state proof public. We need this to read the BABE randomness values from the proof.
Let me know if there is a better way to do this @bkchr