-
Notifications
You must be signed in to change notification settings - Fork 8
Support base64 input too #8
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: main
Are you sure you want to change the base?
Conversation
src/main.rs
Outdated
} | ||
|
||
fn print_parsed_transaction(transaction_payload: SolanaParsedTransactionPayload) { | ||
println!("Solana Parsed Transaction Payload:"); | ||
println!(" Unsigned Payload: {}", transaction_payload.unsigned_payload); | ||
println!( |
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.
It looks like these are some inadvertent formatting changes. Mind reverting these?
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
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 ran cargo fmt
but seems more things changed?
src/main.rs
Outdated
"parse" => { | ||
let unsigned_tx = &args[3]; | ||
let flag = if args.len() > 3 { Some(&args[2]) } else { None }; | ||
println!("Parsing transaction: {}", unsigned_tx); |
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 we can remove some of these prints!
src/main.rs
Outdated
match command.as_str() { | ||
"parse" => { | ||
let unsigned_tx = &args[3]; | ||
let flag = if args.len() > 3 { Some(&args[2]) } else { None }; |
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.
Now with the introduction of format flag
, I'd like to rename this flag
to be a bit more specific.
As I was thinking about it, I was thinking that --message
and --transaction
are both formats, and --hex
and --base64
are types of encodings. So maybe we can rename these variables as such? format_flag
and encoding_flag
respectively
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.
any reason not to use clap
for more consistent parsing?
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.
isn't a message
a component of a transaction? I'd call something a format
if it's representation of same thing. I agree on encoding
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.
if you feel strongly about not adding dependency or have a lighterweight suggestion I can use that, but this is much cleaner to read with clap than manually accounting for indices b6117dc
cc @prasincs — happy to approve + merge once some of these minor bits are taken care of :) thanks again for opening this PR! |
I tried using this library and immediately found that I was converting base64 inputs constantly into hex to pass in. Ideally using a library like clap would've made this easier and less error prone but I like that the library has very few dependencies. I've added an optional
--format
that'shex
by default but you can pass base64 too.It's still backwards compatible with the examples in the README , but you can directly pass the base64 values
returns