Support symbolic amount/bid=everything which spends all funds from selected account/wallets.

[moodyjon]

Fixes: https://github.com/lbryio/lbry-sdk/issues/1628

I know this is lacking in tests for the higher-level functionality like Transaction.pay(), claim_create(), claim_update(), support(). I'm most concerned about claim_update().

However, I haven't been able to get integration tests to run in my environment (MacOS 12.3.1 arm64) and integration testing seems to be the way to work with claims and more complex wallet contents.

I've gone through multiple rounds installing the pieces (scribe, hub, ...) in different ways, but have not been able to get it fully working. Part of the problem is MacOS arm64 does not have a broad selection of pre-built packages at older versions. Part of it is I think I'm still missing pieces of the integration testing environment.

Can someone advise me how to get all the pieces in place for integration testing on my machine or run the integration tests in a supported environment and report back?

[moodyjon]

OK, so I've learned about GitHub actions & workflows, and I'm running that to do testing server-side on the forked repository.

[eukreign]

@moodyjon Thanks for the PR, let me know when you are ready to have it reviewed.

In the meantime I do have a preliminary comment: the --bid cannot be both a decimal and a str, there are programming languages which use the SDK API that have strict typing and decimal/str are not normally interchangeable types.

[moodyjon]

Getting a message in the Actions tab for moodyjon/lbry-sdk repository:

GitHub Actions is currently disabled for your account. Please reach out to GitHub Support for assistance.

Have filed a ticket for it: https://support.github.com/ticket/personal/0/1603371

I'm afraid they believe I'm mining LBC on GitHub servers. Or something like that.

In the meantime, I found scripts/generate_gson_api.py and docs/api.json to be illuminating... though I was not able to run the script locally. I will post a change moving from --bid=everything (str/decimal) to --bid_everything (bool).

[moodyjon]

In my environment (MacOS arm64) I have had partial success running integration tests and success running generate_json_api.py. Ready for review.

No response on ticket: https://support.github.com/ticket/personal/0/1603371

[moodyjon]

GitHub support ticket was resolved, and I can run tests server-side again. Runs are failing, and I'm awaiting the test fix: https://github.com/lbryio/lbry-sdk/pull/3608

[eukreign]

@moodyjon thanks for continuing to work on this, can you rebase on latest master, there were some fixes related to running tests that will improve your test runs

[moodyjon]

@moodyjon thanks for continuing to work on this, can you rebase on latest master, there were some fixes related to running tests that will improve your test runs

Done. Also added one more commit for test issue: https://github.com/lbryio/lbry-sdk/pull/3605/commits/6a82b07f5ffa5cee386433dd838c2caed50d025a

[moodyjon]

@eukreign Do you have your own test runners or are you using plain GitHub runners? Reason I ask is I sometimes get odd timeout or off-by-one errors using the free GitHub runners.

[coveralls]

Coverage remained the same at 57.418% when pulling 4c7b77d1f47c915a4571413fd7f2c4d2ad03af7e on moodyjon:send-max-pr1628 into 7a8d5da0e8df0244bafbfd03acd4403557608dcb on lbryio:master.

[moodyjon]

Review ping to @eukreign or others.

[moodyjon]

Leave the requirement for amount as positional argument, this is the case currently.

Is this a safety concern, or concern about callers using positional arguments? Call sites are still allowed to use bid/amount positionally. I tried to be careful and keep the argument ordering the same, so positional callers would still work.

(lbry-venv) swdev1@Mac-mini lbry-sdk % lbrynet support create cd5d524da448c621144c028b70a0e5b56c7f4768                        
Usage:
            support_create (<claim_id> | --claim_id=<claim_id>) (<amount> | --amount=<amount> | --amount_everything)
                           [--tip] [--account_id=<account_id>] [--wallet_id=<wallet_id>]
                           [--channel_id=<channel_id> | --channel_name=<channel_name>]
                           [--channel_account_id=<channel_account_id>...] [--comment=<comment>]
                           [--preview] [--blocking] [--funding_account_ids=<funding_account_ids>...]
(lbry-venv) swdev1@Mac-mini lbry-sdk % lbrynet support create cd5d524da448c621144c028b70a0e5b56c7f4768 1.0
{
  "code": -32500,
  "data": {
    "args": [],
    "command": "support_create",
    "kwargs": {
      "amount": "1.0",
      "amount_everything": false,
      "blocking": false,
      "channel_account_id": [],
      "claim_id": "cd5d524da448c621144c028b70a0e5b56c7f4768",
      "funding_account_ids": [],
      "preview": false,
      "tip": false
    },
    "name": "InsufficientFundsError",
    "traceback": [
...
    ]
  },
  "message": "Not enough funds to cover this transaction."
}
(lbry-venv) swdev1@Mac-mini lbry-sdk % lbrynet support create cd5d524da448c621144c028b70a0e5b56c7f4768 0.0 --amount_everything
Usage:
            support_create (<claim_id> | --claim_id=<claim_id>) (<amount> | --amount=<amount> | --amount_everything)
                           [--tip] [--account_id=<account_id>] [--wallet_id=<wallet_id>]
                           [--channel_id=<channel_id> | --channel_name=<channel_name>]
                           [--channel_account_id=<channel_account_id>...] [--comment=<comment>]
                           [--preview] [--blocking] [--funding_account_ids=<funding_account_ids>...]

There was one case where I was forced to make an argument optional (claim_id in jsonrpc_stream_repost), but it's only declared that way to Python -- I added a check against None internally to block that.

I would make a counter-suggestion on the safety issue. Do one or more of the following:

1. Reject everything=True when preview=False. People wanting to make use of the feature can extract the amount of the first? output and resubmit the call with the specific amount and everything=false.
2. Reject everything=True when funding_account_ids=None instead of defaulting to all accounts.
3. Have a new configuration knob like max_key_fee, and reject everything=True if the resulting transaction is too valuable.

Internally, instead of having two separate values passed around I think it's safe to convert a combination of amount=0 everything=True coming from API into amount=EVERYTHING (with EVERYTHING="EVERYTHING"). Passing amount=0 without --everything should still be an input error.

I have been working on this today, but it's adding complexity figuring out when amount is valid and converting it back to dewies: int and everything: bool. They came in separately at the higher level in daemon.py. Merging them means they have to be separated again in transaction.py.

I could also be convinced to leave amount=0 internally but my hesitation with that is in case some code internally accidentally passes amount=0 due to some other computation without intending to spend everything (expecting nothing to be spent).

There's currently no special meaning attributed to amount=0 in transaction.py. The everything=True behavior means we augment amount=X (whether X=0 or not) with everything in the funding accounts. Regarding mistakes, I learned there is a way to force everything=True to be passed as a keyword arg NOT positionally. And I could easily add that to improve the safety.

For consistency, all of the --everything flags should be the same for all of the commands for which an amount is defined (instead of the --everything, --amount_everything, --bid_everything, etc)

I am willing to do this, but I thought it was more self-documenting to have --xxx_everything together with xxx. Also they show up together in the alphabetized list of options. Exception was the pre-existing jsonrpc_account_fund --everything which I didn't want to rename.

Checkpointing comment... may return to the amount: Union[int, str] work tomorrow.

[moodyjon]

Implemented the generic amount (int|string) suggestion, and it uncovered places relying on amount being a definite value (int). Fixed these by pulling the amount from the actual "txo" constructed. (8b2c284d7a8a015e0edadc2e39ee8633a5960aaf and 665c12b9b511c7e312031d53dff0c86d8ecb8da1)

Back to review. See my questions/comments.

[moodyjon]

Demote to draft status as I plan to unbundle some changes in subsidiary PRs.

[eukreign]

some of the unbundled PRs have been merged