Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

Improve logging #487

Merged
merged 18 commits into from
Jun 15, 2023
Merged

Improve logging #487

merged 18 commits into from
Jun 15, 2023

Conversation

tonypony220
Copy link
Contributor

@tonypony220 tonypony220 commented May 26, 2023

Usage related changes

Closes #147

  • added --verbose arg
  • added --hide-server-logs arg to hide access logging
  • added --hide-predeployed-contracts arg to substitute --hide-predeployed-accounts
  • added warning to not use it

Development related changes

Checklist:

  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/lint.sh
  • Performed code self-review
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Documented the changes
  • Linked the issues which this PR resolves
  • Updated the tests
  • All tests are passing - ./scripts/test.sh

@FabijanC FabijanC self-requested a review May 26, 2023 13:36
@FabijanC
Copy link
Collaborator

Thanks for the PR! I'll take a look next week.

@FabijanC
Copy link
Collaborator

FabijanC commented Jun 1, 2023

Just to let you know that I didn't forget about this, but currently adapting our tools to the latest cairo-lang/starknet/compiler versions is the highest priority.

@FabijanC
Copy link
Collaborator

FabijanC commented Jun 1, 2023

Btw @tonypony220 I see some of the checkboxes in the PR description aren't checked. Do you plan to address them or do you think they are not necessary?

@tonypony220
Copy link
Contributor Author

Of course! I've done fresh rebase and run tests.

Copy link
Collaborator

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

Why didn't you check the linter checkbox?

starknet_devnet/util.py Outdated Show resolved Hide resolved
starknet_devnet/server.py Outdated Show resolved Hide resolved
page/docs/guide/run.md Outdated Show resolved Hide resolved
page/docs/guide/accounts.md Outdated Show resolved Hide resolved
starknet_devnet/account.py Outdated Show resolved Hide resolved
starknet_devnet/util.py Outdated Show resolved Hide resolved
starknet_devnet/util.py Outdated Show resolved Hide resolved
starknet_devnet/util.py Show resolved Hide resolved
starknet_devnet/udc.py Outdated Show resolved Hide resolved
starknet_devnet/starknet_wrapper.py Outdated Show resolved Hide resolved
@tonypony220
Copy link
Contributor Author

Should I make all conversations resolved?

@FabijanC
Copy link
Collaborator

FabijanC commented Jun 12, 2023

Should I make all conversations resolved?

@tonypony220 Hey, sorry for not having replied yet. Don't resolve the conversations, I plan to take a look at your new commits. It's just that I'm currently busy on other fronts.

@FabijanC
Copy link
Collaborator

Just now I realized that this PR is still introducing a breaking change:

changes args --hide-predeployed-accounts to --hide-predeployed-contracts

Do you have an opinion on that? I really wouldn't want to bump to 0.6.0 just because of this

starknet_devnet/blueprints/gateway.py Outdated Show resolved Hide resolved
log Outdated Show resolved Hide resolved
starknet_devnet/blueprints/rpc/transactions.py Outdated Show resolved Hide resolved
starknet_devnet/devnet_config.py Show resolved Hide resolved
starknet_devnet/devnet_config.py Outdated Show resolved Hide resolved
starknet_devnet/fee_token.py Show resolved Hide resolved
starknet_devnet/starknet_wrapper.py Show resolved Hide resolved
starknet_devnet/starknet_wrapper.py Outdated Show resolved Hide resolved
@FabijanC
Copy link
Collaborator

Also a reminder to update the PR description.

page/docs/guide/run.md Outdated Show resolved Hide resolved
starknet_devnet/devnet_config.py Outdated Show resolved Hide resolved
starknet_devnet/devnet_config.py Outdated Show resolved Hide resolved
@FabijanC
Copy link
Collaborator

FabijanC commented Jun 14, 2023

Were you planning to make more changes? I just ran your code and noticed the following issues:

  • the warning WARNING: Use these accounts and their keys ... is logged twice
  • chargeable account info is logged - let's not do that, it's an undocumented testing-thing only
  • predeclared starknet cli account class hash is logged in decimal instead of hexadecimal format (also, let's write Starknet CLI instead of starknet cli - it looks better IMO
  • thinking about it, the name --hide-logs is confusing because every part of output can be considered logging; perhaps rename to --hide-server-logs to avoid confusion
  • in verbose mode, logging the compressed program brings no benefit IMO; e.g. for my very simple test contract, it logged a the whole ABI pretty-printed and a wall of content like this: H4sIAKiQiWQC/+y9C5PktpUu+Fc6FLHhx3q68X447myEbPfMOK78WEmzc+/OODJAEFTXuiqrVJklqceh/75gZj0yyWQSJEEQJGGH3d1VJAgcfPjOwes7//hC7...

Copy link
Collaborator

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

I'll assume you're done with new commits and approve&merge.

@FabijanC FabijanC merged commit 0d5d69d into 0xSpaceShard:master Jun 15, 2023
FabijanC added a commit that referenced this pull request Jun 16, 2023
 * Keeps the features of the PR but restores the log format

 * [skip ci]
FabijanC added a commit that referenced this pull request Jun 16, 2023
* Keeps the features of the PR but restores the log format
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve logging
2 participants