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

Improve logging API and usage in the cmd package #300

Open
anishnaik opened this issue Feb 20, 2024 · 3 comments
Open

Improve logging API and usage in the cmd package #300

anishnaik opened this issue Feb 20, 2024 · 3 comments

Comments

@anishnaik
Copy link
Collaborator

          Yeah probably. I've had a thought while reviewing some logging code that some of the CLI entry point logging could use a bit of cleanup and maybe our logging API should be reworked so the Info/Error/other log functions maybe are a bit more intuitive. I have some ideas we can follow up on in another PR anyways, so I'm probably less concerned about this.

But yeah idk, my thoughts are here, maybe it makes sense to log to it in case we access the logger elsewhere or for some other purpose (would this even be useful to us if we did?). 🤷

Originally posted by @Xenomega in #216 (comment)

@Xenomega
Copy link
Member

Xenomega commented Feb 20, 2024

More loose thoughts:

  • We can probably make printf style logging if we wrap colors in an intermediate structure and disable/enable the values depending on context provided (if the logging faucet supports color or not).
  • In the event of errors, a stack trace during panic is not always included, but it probably makes sense that kind of info to be logged in a panic, it's more interesting to have in logs than other data.
  • Some of the verbose output including module maybe isn't worth keeping if we don't expand this, especially if we have stack traces for errors.
  • The main entry points (CLI/cmd) duplicates some error messages we could probably wrap. e.g. We log an error, then return a different one that looks similar.
  • Some coloring things: maybe not worth coloring [ and ] brackets on statuses.
  • There was an error outputting I recently encountered that kept saying "nil" pretty repetitively. I need to hunt this down again and I'll share here.

@anishnaik
Copy link
Collaborator Author

anishnaik commented Feb 20, 2024

In the event of errors, a stack trace during panic is not always included, but it probably makes sense that kind of info to be logged in a panic, it's more interesting to have in logs than other data.

For error stack traces, we can use this PR and refactor it:
#108

We cannot use the standard library's errors and instead have to import pkg/errors.

Some of the verbose output including module maybe isn't worth keeping if we don't expand this, especially if we have stack traces for errors.

Agreed

The main entry points (CLI/cmd) duplicates some error messages we could probably wrap. e.g. We log an error, then return a different one that looks similar.

This has also introduced a bug because if you set NoColor to true, the CLI output from the cmd package will still be colorized bc we haven't processed the config values yet.

@aviggiano
Copy link

Hey

Another request would be to format addresses with Mixed-case checksum encoding (ERC-55)

Right now, copy/pasting Medusa's logs to Solidity does not compile. Example:

1) CryticTester.deposit(address,uint256)(0x470c8b323b89c05bbb0f2a85d733cc7004883d14, 45) (block=2, time=159480, gas=12500000, gasprice=1, value=0, sender=0x0000000000000000000000000000000000010000)
2) CryticTester.withdraw(address,uint256)(0x00000000000000000000000000000000002fef7c, 70880928519027259232681487313122362188527844215780522898084047764600190669520) (block=12543, time=672109, gas=12500000, gasprice=1, value=0, sender=0x0000000000000000000000000000000000010000)
    function test_CryticToFoundry_12() public {
        deposit(0x470c8b323b89c05bbb0f2a85d733cc7004883d14, 45);
        withdraw(0x00000000000000000000000000000000002fef7c, 70880928519027259232681487313122362188527844215780522898084047764600190669520);
    }
This looks like an address but has an invalid checksum. Correct checksummed address: "0x470c8B323b89c05bbB0F2a85d733Cc7004883d14". If this is not used as an address, please prepend '00'. For more information please see https://docs.soliditylang.org/en/develop/types.html#address-literalssolidity(9429)

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

No branches or pull requests

3 participants