-
Notifications
You must be signed in to change notification settings - Fork 4
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
app: more logs for mining, wallet updates, net #16
Conversation
1aa232b
to
de87372
Compare
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.
Mostly LGTM, requested some style/code quality changes
tracing::debug!(%bmm_txid, "mine: confirming BMM..."); | ||
if let Some((main_hash, header, body)) = | ||
miner_write.confirm_bmm().await? | ||
{ | ||
tracing::debug!( | ||
"mine: confirmed BMM, submitting block {}", | ||
header.hash() | ||
%main_hash, side_hash = %header.hash(), "mine: confirmed BMM, submitting block", |
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.
Nit: There is no need to prefix logs with the function name. The logger should do that automatically. You can use tracing::instrument
with skip_all
if you want to construct a new span specific to the function.
Re. all the whitespace: we probably look at this differently. I think whitespace is a good way of logically chunking groups of code lines that "belong together", and avoiding whitespace quickly leads to walls of text that's tiring to read. I'd be fine with a strict standard for whitespace, if it's something we can automate through |
Re. the log lines including function names: that's not the case when I'm running locally. Is that perhaps something that needs to be configured? Providing example logs here
|
5a49687
to
c1b2950
Compare
No description provided.