-
Notifications
You must be signed in to change notification settings - Fork 2
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
Jumpstart network #918
Jumpstart network #918
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.
There's a couple more things I need to look at, in particular the use of the zero-address around the different calls
pallets/registry/src/lib.rs
Outdated
})] | ||
pub fn jump_start_network(origin: OriginFor<T>) -> DispatchResult { | ||
let _who = ensure_signed(origin)?; | ||
let network_account = H256::zero(); |
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 may want to avoid the zero address here because it has a well-known private key.
I say may instead of must since I don't have a full idea of where this address is actually "used", or if it's more of just a placeholder.
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 be easily fixed with a guard on register, good find
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Ive had a quick look at this and it looks great. Since it is a step towards having a network parent key but not fully implemented, Im trying to figure out what exactly the scope of this PR is. This is what i am getting, please correct if wrong:
|
yes
also yes because we don't use it yet.....and as we move the registery flow to the new flow this, is where having a block before jumpstart should be addressed
No it is all of them...... If you are referring to this line https://github.com/entropyxyz/entropy-core/pull/918/files#diff-1e0acd338c23affee5840b8efb06bd46be87c49819841bdcf3ce804631a40b92R340 it is because the last confirmation sets it to done as if there were 4 out of 5 done, this one is the 5th
|
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.
Getting close!
Couple things:
- It's unclear to me why we need to substitute the parent verifying key in the KVDB with
0x00...
- I'm still slightly worried about the use of the zero address here, but we could kick this down the road to at least get things moving (maybe we open an issue for right now)
- We need to rename comments, errors, etc. to
parent
ensure!( | ||
encoded_sig_req_account != NETWORK_PARENT_KEY.encode(), | ||
Error::<T>::NoRegisteringFromParentKey | ||
); |
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 this could be useful to have. E.g we can have the Entropy network control an account on other chains like this.
Obvs this wouldn't work if the parent key address is the zero-address though.
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
In theory we don't and we can change it, but when we keep it a set amount, we can and it is easier to make it a global constant and have if parent key do x code easier (which we can also add extra security on it too)
Global constant, I know your impulse, I think it is fine we can make it any random key tbh
|
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.
Few things we need to follow up on, but looks good otherwise!
data.block_number, | ||
) | ||
.await?; | ||
let verifying_key = key_share.verifying_key().to_encoded_point(true).as_bytes().to_vec(); | ||
let string_verifying_key = hex::encode(verifying_key.clone()).to_string(); | ||
let string_verifying_key = if sig_request_account == NETWORK_PARENT_KEY.encode() { | ||
hex::encode(*NETWORK_PARENT_KEY) |
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.
One suggestion: instead of overwriting the parent's verifying key to be 0x00
we could
have a separate "forbidden key" entry, PARENT_VERIFYING_KEY
, which points to the parent
verifying key.
For those checks you mention, we could achieve them the same way, but doing an extra
lookup.
That way we keep the verifying key entries consistent.
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.
sure at this point tho im in favour of opening an issue
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 an issue is fine
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.
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Jumpstarts the network with a global key