-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add genesis config hash as extra data to migration ready transaction #2479
base: albatross
Are you sure you want to change the base?
Conversation
9910539
to
eaab34c
Compare
"Generating ready transaction" | ||
); | ||
OutgoingTransaction { | ||
from: validator, | ||
to: Address::burn_address().to_user_friendly_address(), | ||
value: 1, //Lunas | ||
fee: 0, | ||
data: None, | ||
data: Some(hash.to_hex()), |
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.
Which is the reason why we want this in hex
rather than the raw hash?
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.
The PoW RPC specs expects OutgoingTransaction.data
to be a String
. A raw Blake2bHash
doesn't implement that. I could have gone for to_string
but to_hex
is something we control and have defined which is a little more consistent.
// Here we filter by current epoch | ||
// Check if the txn contains extra data and matches our genesis block hash | ||
if let Some(txn_data) = &txn.data { | ||
if *txn_data != genesis_config_hex { |
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.
Same here, why in hex
?
// Here we filter by the readiness criteria, TBD | ||
// Check if the txn contains extra data and matches our genesis block hash | ||
if let Some(txn_data) = &txn.data { | ||
if *txn_data != genesis_config_hex { |
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.
Same here, why in hex
?
let filtered_txns: Vec<TransactionDetails> = transactions | ||
.into_iter() | ||
.filter(|txn| { | ||
// Here we filter by current epoch | ||
// Check if the txn contains extra data and matches our genesis block hash | ||
if let Some(txn_data) = &txn.data { |
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'd suggest to factorize the txn data check in a function to avoid code duplication
@@ -111,7 +127,16 @@ pub async fn check_validators_ready( | |||
let filtered_txns: Vec<TransactionDetails> = transactions | |||
.into_iter() | |||
.filter(|txn| { | |||
// Here we filter by the readiness criteria, TBD | |||
// Check if the txn contains extra data and matches our genesis block hash | |||
if let Some(txn_data) = &txn.data { |
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'd suggest to factorize the txn data check in a function to avoid code duplication
eaab34c
to
d46b911
Compare
What's in this pull request?
Pull request checklist
clippy
andrustfmt
warnings.