-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Added checksum to eth address, added teleburn to inscriptions page #1825
Conversation
This looks pretty fun! |
@@ -10,35 +11,10 @@ pub struct Output { | |||
ethereum: EthereumTeleburnAddress, | |||
} | |||
|
|||
#[derive(Debug, PartialEq)] |
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.
test comment plz ignore
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.
Review ACK
This reviews suggested changes are to "plz ignore" and that is what I do with code reviews anyways.
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 really love this work, but just one question.
fn from(inscription_id: InscriptionId) -> Self { | ||
let digest = bitcoin::hashes::sha256::Hash::hash(&inscription_id.store()); | ||
Self { | ||
address: create_address_with_checksum(&digest[0..20].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.
Wouldn't it make more sense to just use md160 as the hash function here? It's the same one that hashes public keys in P2WPKH addresses, and it also produces 20 bytes.
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 an implementation that @casey and rob did thats already been used to teleburn some NFTs, so i stuck with this to not invalidate that burn
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.
Gotcha, well, this works too!
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 like it. Just gonna merge this into the teleburn
branch so we can continue discussing there.
In this PR: