Implement 'max' value send

[cliik]

This PR adds a special value 'max', which can be accepted as a spend amount. If this amount is specified, the wallet will calculate the maximum spendable amount (less fees) and send that amount. This is effectively a sweep.

PR is currently in draft state, as I need to test and think through edge cases some more.

Resolves https://github.com/mimblewimble/grin-wallet/issues/601

[cliik]

I've taken the time to test the following scenarios, and all worked as expected:

• normal send/receive functionality -- still works as before :heavy_check_mark:
• send max with zero coins in wallet -- failed gracefully with insufficient funds error :heavy_check_mark:
• send max with only unspendable coins in wallet -- failed gracefully with insufficient funds error :heavy_check_mark:
• send max with mix of spendable/unspendable -- correctly computed fee & spent available coins :heavy_check_mark:
• send max with only spendable coins -- correctly computed fee & spent entire balance with no change :heavy_check_mark:

I cannot think of any other cases to test and I feel comfortable marking this ready for review. cc @yeastplume

[cliik]

@quentinlesceller the windows tests failed for a reason that appears to be unrelated to this feature:

thread 'slatepack_api' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 32, kind: Uncategorized, message: "The process cannot access the file because it is being used by another process." }', controller\tests\common\mod.rs:79:50

Can you trigger the workflow to run again, to see if it was a transient failure?

[cliik]

Can you trigger the workflow to run again, to see if it was a transient failure?

I decided to force-push to trigger a rerun. We'll see if Windows tests fail again :crossed_fingers:

[davidtavarez]

I was looking for this feature recently! @cliik would you mind to write some tests for this?

[yeastplume]

The implementation looks good and seems to work well. The wallet has extensive tests to ensure as much is covered as possible and it keeps working well, so the next step would be to automate all of the tests you've outlined above. Here's the general process:

• Start an integration test by copying one of the existing test files in controller\src\tests. These simulate wallet-to-wallet and wallet-to-node communication so you should be able to add tests to cover all of the new cases you outlined above and check the results.
• Check the API tests to make sure the samples don't need adjustment. In this case, send_args aren't really used in the doctest examples, so I don't think there's much further to do here as long as the InitTxArgs field is documented.

When adding a feature, I usually start by developing one of the tests in the controller\src\tests director to exercise it, which also saves a lot of development time cause I don't need to manually run wallets and check the results. Also easy to add new test cases there to catch explicit conditions if a bug or issue comes up.

[cliik]

@davidtavarez @yeastplume I've spent several hours trying to get automated tests working for the CLI, but hitting a roadblock. Can you point me to any tests that exercise the wallet commands, instead of the wallet API? So far all I see are tests for the wallet API, which is not useful for this feature, because the change does not exist in the API methods.

Alternatively, I could re-implement this feature in the init_send_tx method of the owner API instead of in the CLI command processing. Originally, I didn't want to make an API change, because this feature is likely not useful for API users, and I didn't want to cause a breaking change to the API for a feature API users won't want... perhaps my choice was wrong, though.

Pros/Cons to moving this into the owner API:

• pro -- easy to write tests around API
• pro -- more similarity between API and CLI options
• con -- breaking API change

[yeastplume]

I'm of two minds as well as to whether this should be in the API, I think it may be fine to leave it as is for now and we can revisit later. If it's just adding an argument to the send args it shouldn't break any existing APIs, but it would still need more thought and consideration than placing it where you have it now.

For testing at the CLI level, look in the top level src\tests\cmd_line_basic.rs for examples

[davidtavarez]

@cliik @yeastplume what do you of instead of adding a parameter the API can receive amount as null and that will means to use max; that's how others API are doing this. Just an idea.

[cliik]

@yeastplume ahhthanks! Idk why it was so hard for me to find those CLI tests. I'll take a look later/tomorrow.

@davidtavarez in general I prefer to be explicit. If someone accidentally omits/nulls an argunent, the wallet shouldn't proceed to empty their coins.

If I was to move this to the API (which I'm starting to become in favor of), it might make more sense to change InitTxArgs.amount to an enum with two variants. One variant being the usual specified amount, and another variant being the max spend option. This would be clean & simple, and also opens the door for other variants in the future (i.e. if we wanted a variant to spend X% balance or something like that)... I dont think we should feature creep here, lol, but future extensibility is an additional benefit worth mentioning.

[cliik]

Ok, I've thought about this feature some more and I'm not happy with my original approach. After brainstorming a few better solutions, I also don't like using an enum with special variants for the amount. That would have rippling consequences in several other parts of the codebase, and the changes would be messy at best.

I'm going to close this PR for now, and discuss the new approach I have in mind in the original issue: https://github.com/mimblewimble/grin-wallet/issues/601

[cliik]

This PR has been replaced by a better approach (IMO): https://github.com/mimblewimble/grin-wallet/pull/657