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
Creates a default address for the sweep_all command in the cli wallet #1031
Conversation
Previously the sweep all command required the user pass in the wallet address for it to function. This commit allows for no address to be passed in which case the software will use the same address that the address command produces.
src/simplewallet/simplewallet.cpp
Outdated
{ | ||
default_addr = local_args[0]; | ||
} | ||
if (!cryptonote::get_account_address_from_str_or_url(info, m_wallet->nettype(), default_addr, oa_prompter)) |
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.
This is nice - and it matches up with the GUI wallet's ability to sweep all to myself without putting in an address (I don't use the GUI wallet often, but as I recall there's a button on the staking page that can do this).
A few comments:
- the test for whether a wallet was passed should be using
local_args
rather thanargs_
. (local_args
has had all the other things that it understood pulled off such as priority,index=
,outputs=
arguments). That way I can specify something likesweep_all unimportant
to do a lowest priority sweep to myself.) default_addr
is sort of a weird name because it starts out as the default address but then, if there's an argument, gets set to the (non-default) argument.- There's no need to fetch the current wallet address with the
get_subaddress...()
call if there is a wallet address specified (since you're just going to replace it). Better to only make the call only if you need it with something like:
std::string addr;
if (local_args.size() > 0)
addr = local_args[0];
else
addr = m_wallet->get_subaddr...;
- The usage and descriptions need an update to make the feature discoverable, i.e. putting
[
and]
around the address and payment ID in the usage to show it as:[<address> [<payment_id (obsolete)>]]
in the usage string, and then describe that it sweeps to your main address if omitted in the descriptions. (Also note that this change affects both sweep_all and sweep_below, so both need an update!).
Thanks for the review, updated according to your comments. I also noticed that the optional address would mess with the locked_sweep_all command which requires the 2 parameters so updated that part of the code aswell |
loki-wallet-cli.log
Outdated
@@ -0,0 +1 @@ | |||
2020-02-01 07:42:15.137 7f6254a7e400 INFO logging contrib/epee/src/mlog.cpp:273 New log categories: *:WARNING,net:FATAL,net.http:FATAL,net.ssl:FATAL,net.p2p:FATAL,net.cn:FATAL,global:INFO,verify:FATAL,serialization:FATAL,logging:INFO,msgwriter:INFO |
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.
I don't think you meant to add this file
src/simplewallet/simplewallet.cpp
Outdated
const char* USAGE_LOCKED_SWEEP_ALL("locked_sweep_all [index=<N1>[,<N2>,...]] [<priority>] [<address>] <lockblocks> [<payment_id (obsolete)>]"); | ||
const char* USAGE_SWEEP_ALL("sweep_all [index=<N1>[,<N2>,...]] [blink|<priority>] [outputs=<N>] [<address> [<payment_id (obsolete)>]] [use_v1_tx]"); | ||
const char* USAGE_SWEEP_BELOW("sweep_below <amount_threshold> [index=<N1>[,<N2>,...]] [blink|<priority>] [<address> [<payment_id (obsolete)>]]"); | ||
const char* USAGE_SWEEP_SINGLE("sweep_single [blink|<priority>] [outputs=<N>] <key_image> [<address> [<payment_id (obsolete)>]]"); |
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.
I don't think the code path you changed applies to sweep_single (and probably doesn't need to).
More updates, removing the log file that was incorrectly submitted and sweep_single didn't actually call sweep_main so removed the instructions |
This looks pretty good but needs one more small change: there is a text description that shows up when you run |
Yep good point. I copied the description used in |
…oxen-io#1031) * Creates a default address for the sweep_all command in the cli wallet Previously the sweep all command required the user pass in the wallet address for it to function. This commit allows for no address to be passed in which case the software will use the same address that the address command produces. * Rename variable to addr, only call wallet if needed and update usage statements * modify locked blocks parameters after making address optional * removed log file and single sweep description * updated the help descriptions for the 3 affected commands
addresses #928
Previously the sweep all command required the user pass in the wallet
address for it to function. This commit allows for no address to be
passed in which case the software will use the same address that the
address command produces.