Skip to content
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

Feat: add log hints to integers parsed in verbose output #4745

Merged

Conversation

merklefruit
Copy link
Contributor

Motivation

Added log hints on Uint values printed by the logger converted in Ether.

Closes #4743

Solution

I added a simple helper: format_uint_with_ether_conversion which appends a dimmed [x ether] text at the end of the parsed Uint.

I added some heuristics to try and make the hints more useful for debugging: the conversion is skipped if the amount is 0, if it's less than 0.0001 ether or more than 1 million ether.

Notes

  • I think log hints could make the debugging experience nicer, and this is just a small step in that direction
  • Should there be the possibility to show these hints only on a specific verbosity level or to turn them off completely?

Preview

Here is a preview of what it looks like:

example_eth_1

I've also tested it on uniswap/permit2 to see how it would look like on a larger codebase. Here's a test taken from that repo:

example_permit2

@mds1
Copy link
Collaborator

mds1 commented Apr 16, 2023

This seems nice, I like it, thanks! What do you think of using exponents instead, e.g. showing 1e18 instead of 1 ether? I like that better for a few reasons:

  1. It's shorter, so adds less clutter
  2. Not all numbers are actually ether amounts, so it might confuse users
  3. It's more extensible to e.g. smartly choosing exponents or letter a user configure the exponent, e.g. if they're using USDC you can imagine a trace config (meta(tracing): tracking issue for tracing improvements #3390) where the user chooses to use e6 instead of e18

@merklefruit
Copy link
Contributor Author

merklefruit commented Apr 16, 2023

Configuration for traces sounds like a really cool feature! Definitely looking forward to it.

I like the idea of shorter exponents, I'll work on it asap.
Just few questions to better understand what you have in mind:

if we have a number like 999999999999999999000000000000000000 what should we show?

  • [9.9e35], if so with how many digits of precision?
  • [999999999999999999e18]
  • keep the current heuristics to hide numbers greater or smaller than a sensible threshold

@mds1
Copy link
Collaborator

mds1 commented Apr 16, 2023

Good question, off the cuff I'd say 4-5 significant figures is probably a sane default to keep it simple for now, since you can always look directly to the left to see the full number with full precision, using your example I think [9.999e35] would be useful

@merklefruit merklefruit changed the title Feat: add log hints to add [x ether] to Uints parsed in verbose output Feat: add log hints to integers parsed in verbose output Apr 17, 2023
@merklefruit
Copy link
Contributor Author

Added exponential notation hints and updated PR title :)

common/src/abi.rs Outdated Show resolved Hide resolved
common/src/abi.rs Outdated Show resolved Hide resolved
common/src/abi.rs Outdated Show resolved Hide resolved
common/src/abi.rs Outdated Show resolved Hide resolved
common/src/abi.rs Outdated Show resolved Hide resolved
common/src/abi.rs Outdated Show resolved Hide resolved
merklefruit and others added 3 commits April 18, 2023 19:32
take by value

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@merklefruit
Copy link
Contributor Author

I moved the helper function to the calc.rs file and I added more comments, docs and a few test cases

@merklefruit merklefruit requested a review from mattsse April 26, 2023 16:35
@mattsse mattsse added the C-forge Command: forge label Apr 27, 2023
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

smol nit, otherwise lgtm

common/src/abi.rs Show resolved Hide resolved
common/src/calc.rs Outdated Show resolved Hide resolved
@merklefruit
Copy link
Contributor Author

Fixed some CI tests - should be good now

@mattsse
Copy link
Member

mattsse commented May 9, 2023

whoops, sorry totally forgot about this!

@mattsse mattsse merged commit 7bba788 into foundry-rs:master May 9, 2023
cruzdanilo added a commit to exactly/protocol that referenced this pull request May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log hints in verbose output
3 participants