-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
add balance command #1047
add balance command #1047
Conversation
Nice! Don't worry about rebasing. I'll just squash at the end. I'm leaning towards displaying the balance in sats:
What do you think? This could use an integration test in |
e89d144
to
ef20a9f
Compare
ended up doing a rebase anyway. |
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.
Looking good, a few minor comments.
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.
vv minor changes
tests/wallet.rs
Outdated
.expected_stdout("0\n") | ||
.run(); | ||
|
||
let _ = &rpc_server.mine_blocks(1); |
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.
You can just do:
let _ = &rpc_server.mine_blocks(1); | |
rpc_server.mine_blocks(1); |
let _ =
is only needed when ignoring a type that has #[must_use]
on it, like Result
.
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.
right you are
tests/wallet.rs
Outdated
|
||
CommandBuilder::new("--regtest wallet balance") | ||
.rpc_server(&rpc_server) | ||
.expected_stdout(format!("5000000000\n")) |
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.
Since this doesn't have any interpolations, format is unnecessary:
.expected_stdout(format!("5000000000\n")) | |
.expected_stdout("5000000000\n") |
There's a just recipe for running some of the checks that run on ci, try just ci
, or look for the ci recipe in the justfile to find the commands to run. CI runs clippy (which would complain about this), checks formatting, and runs bin/forbid, which forbids certain strings from appearing in the codebase.
bin/forbid is actually quite useful. It forbids the string todo
, so if you want to make sure you don't forget something before comitting, write todo: I need to do blah blah blah
and CI will complain, so you don't need to worry about forgetting it.
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.
ah! I haven't used just
before. I'll check that out so you don't have to keep commenting on things that the linter would check. thanks!
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.
No worries! just is kind of like make, but only for saving and running commands, and less weird.
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.
yeah, this is great. should be a lot easier to make sure im conforming to style, etc. for future PRs :)
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.
LGTM!
Add a simple balance command. Here's what it looks like:
closes #1036