Skip to content

Conversation

@kopy-kat
Copy link

@kopy-kat kopy-kat commented Sep 23, 2023

Show stack output: some comments are quite long and cumbersome to read (eg call(param1, param2, ..., param7) so users can provide an optional --stack-output flag to override these with alternative values (in this case success since that is what is pushed to the stack after a call)

src/opcodes.rs Outdated
"invalid" => INVALID,
"selfdestruct" => SELFDESTRUCT,
&_ => panic!("Unknown opcode: {}", name),
&_ => JUMPLABEL,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

treating anything else as a jump label feels wrong to me. we should probably parse it out correctly. maybe use the JumpLabelMap for that

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - this was more of a hotfix since otherwise it would error out. Will check out JumpLabelMap

@shafu0x
Copy link
Owner

shafu0x commented Sep 23, 2023

Great!

Lets split this into 2 PRs. I think the alt comments we can merge in.
I think we can deal with the JumpLabels in a better way.

src/opcodes.rs Outdated
pub pops: usize,
pub pushes: usize,
pub sign: Option<&'static str>,
pub alt: &'static str,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we use a more descriptive term for "alt"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas? Maybe something related to "return value" since it adds that to the stack rather than the entire function call

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to stack output and show stack output (in line with evm.codes terminology) - wdyt @shafu0x

@kopy-kat kopy-kat changed the title Add jumplabels hack and alt comments Add alt comments Sep 23, 2023
@kopy-kat
Copy link
Author

Jumplabels hack removed from the pr

@kopy-kat kopy-kat changed the title Add alt comments feat: add show stack output option Sep 24, 2023
show-stack-output: [--stack-output]
```

Note: this setting will add stack outputs to the stack where possible. For example, instead of adding `call(param1, param2, ..., param7)` it will add `success` to the stack comments.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we show both states of possible outcomes. So success | failure or 0 | 1. Because we don't actually know if it fails or not.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is currently in line with meth-weth and evm.codes but happy to change it if you think success | failure is more descriptive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants