-
Notifications
You must be signed in to change notification settings - Fork 260
feat(txscript): introduce human readable viewer #743
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
base: master
Are you sure you want to change the base?
feat(txscript): introduce human readable viewer #743
Conversation
|
|
||
| macro_rules! opcode_list { | ||
| ( $( opcode $(|$alias:ident|)? $name:ident<$num:literal, $length:tt>($self:ident, $vm:ident) $code: expr ) *) => { | ||
| ( $( opcode $(|$alias:ident|)? $name:ident<$str_rep:literal, $num:literal, $length:tt>($self:ident, $vm:ident) $code: expr ) *) => { |
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 $name:ident can be used instead on str_rep directly and it will be consistent with enum
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.
Ok, let's try this 👍
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'm not familiar with macros, but compiler complains on introduced $name:ident with error duplicate matcher binding
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.
name:ident already there. You can reuse it
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 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.
No
You are trying to introduce new one with the same name. Reusing it means remove the newly added variable and use the first "name"
| #[derive(Debug)] | ||
| pub struct MultiSigScriptParameters { | ||
| pub required_signatures_count: u8, | ||
| pub signers_count: u8, | ||
| pub signers_pubkey: Vec<secp256k1::XOnlyPublicKey>, | ||
| } |
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.
that is only correct for schnorr based multisigs. so either add enum or suffix to make it cleat that it can only work with schnorr based mulisigs
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.
in review: make it clear it only support schnorr multiscript, precised naming to restraint to schnorr.
It seems to be standard, as the opcode OpCheckMultiSig is for Schnorr, while this one: OpCheckMultiSigECDSA is specifically for ECDSA
crypto/txscript/src/viewer.rs
Outdated
| s.push(' '); | ||
| s.push_str(&hex::encode(data)); | ||
|
|
||
| // try to disassemble the data as a script |
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.
maybe it needs explicit param to try disassemble. also output is not clearly specifies that redeem script and last push data are the same. maybe it makes sense to wrap it into : (redeem script) or something like that
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.
maybe it makes sense to wrap it into
: (redeem script)
Could you please describe a bit more what you mean by that?
I'm not sure to get it properly. Thanks :)
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 meant formatting.currently it looks like
OpCode, OpCodeData data
Redeem script
There's no direct link between data and redeem script. I suggest adding some extra delimiters to make it look like OpData hex: (redeem script)
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.
Makes sense, on it
| borsh.workspace = true | ||
| bs58.workspace = true | ||
| cfg-if.workspace = true | ||
| hex.workspace = true |
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.
use faster hex. i think we should use the library everywhere instead of mixing them
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.
addressed in review: replace hex with faster hex for fresh code
|
As per requested by @someone235 and @biryukovmaxim, added a parameter to enable sub script disassembling. If false, it will not attempt disassembling redeem script: review: add parameters to enable subscript disassembling |
|
Converted to draft in the meantime of a cleaner implementation. (cf. review by Maxim) |

Usage:
Input:
Output:
An early review has been made by @biryukovmaxim here for the record. Thanks Maxim 🙏