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

moodyjon · 2022-04-26T15:15:52Z

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 · 2022-04-26T18:38:06Z

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

eukreign · 2022-04-27T14:48:33Z

@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 · 2022-04-28T13:29:19Z

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 · 2022-05-05T01:27:35Z

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 · 2022-05-11T13:44:42Z

GitHub support ticket was resolved, and I can run tests server-side again.
Runs are failing, and I'm awaiting the test fix:
#3608

eukreign · 2022-05-18T14:30:54Z

@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 · 2022-05-18T17:34:23Z

@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: 6a82b07

moodyjon · 2022-05-18T17:36:50Z

@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 · 2022-05-26T18:20:22Z

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

moodyjon · 2022-06-02T15:26:12Z

Review ping to @eukreign or others.

eukreign

Thanks for putting a lot of effort into this PR.

I'm struggling with a clean way to handle the --everything case without compromising the existing functionality... what about this:

Leave the requirement for amount as positional argument, this is the case currently. Then have the user enter 0 as the amount followed by the --everything flag to achieve the spending of all UTXOs. Since spending --everything is a significant operation, perhaps requiring an extra step of both amount=0 and --everything is not too bad. (Kind of like on github when you delete something important they ask you to enter a string into a text box.) This also solves the issue of not removing the positional argument.

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 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). By making it a string it reduces any inadvertent computation of amount variable without a if amount == EVERYTHING check (since things will break when doing math with strings thus preventing unintended/unguarded spends). Although another option for this particular case is to use -1 as --everything... on the other hand, one other advantage using "EVERYTHING" as a string is that in cases where an unrelated error message is printed but if it includes amount=EVERYTHING as part of the error output it will actually make sense to the user because that is what they intended.

In summary,

API

Leave existing API for positional amounts argument unchanged.
In --everything doc state that user must specify 0 for the amount.
Require user to enter 0 for amount following by --everything when they want to spend everything and check this in daemon.py.
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)

Internally

Convert the user input of amount=0 everything=True into amount=EVERYTHING (you can define EVERYTHING="EVERYTHING" in wallet/constants.py) to reduce number of arguments being passed around everywhere.

What do you think?

tests/unit/wallet/test_transaction.py

…arg. (Derived from 33bdb50 but without test code.)

…hing). Make bid and amount args optional as well as args following them. Fold agrument checks into get_dewies_or_error().

…mands.py.

…_json_api.py.

…s_or_error().

…in daemon.py. Unpack the generic amount in transaction.py and account.py. Eliminate separate "everything" keyword args.

…ts().

moodyjon · 2022-06-09T17:40:41Z

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. (8b2c284 and 665c12b)

Back to review. See my questions/comments.

eukreign

I think it's getting closer but it's a big PR and would be great if anything extraneous could be moved into a separate PR and then to just limit the number of changed lines in this one to make it easier to review. See a bunch of inline comments below.

eukreign · 2022-07-11T13:01:51Z

lbry/extras/daemon/daemon.py

        """
        Send the same number of credits to multiple addresses using all accounts in wallet to
        fund the transaction and the default account to receive any change.

        Usage:
-            wallet_send <amount> <addresses>... [--wallet_id=<wallet_id>] [--preview]
+            wallet_send (<amount> | --amount=<amount> | --amount_everything) <addresses>...


I don't think you can have an optional followed by a required field. When calling wallet_send here it's not clear if the first value passed to the command would be an <amount> or an <address>.

eukreign · 2022-07-11T13:02:28Z

lbry/extras/daemon/daemon.py

                        [--change_account_id=None] [--funding_account_ids=<funding_account_ids>...]
                        [--blocking]

        Options:
+            --amount=<amount>               : (decimal) the amount to transfer lbc
+            --amount_everything             : (bool) send everything from funding accounts (excluding claims),


Can this just be --everything?

eukreign · 2022-07-11T13:13:57Z

lbry/wallet/transaction.py

+            all_utxos = []
+            for acct in funding_accounts:
+                # TODO: Constraints for get_utxos()?
+                utxos = await acct.get_utxos()


UTXOs will include claims

On the jsonrpc_account_fund() code path, it seems the only extraneous arg which could be interpreted as a constraint is from_account=None. Will investigate this...

Possibly this imposes a constraint to exclude claims?

lbry-sdk/lbry/wallet/ledger.py

Line 958 in dc427ec

def constraint_spending_utxos(constraints):

TXO_TYPES = { "other": 0, "stream": 1, "channel": 2, "support": 3, "purchase": 4, "collection": 5, "repost": 6, } def constraint_spending_utxos(constraints): constraints['txo_type__in'] = (0, TXO_TYPES['purchase'])

More context:
7d333ef

I take it "other" is a generic txo carrying an amount of LBC created by wallet_send, and "purchase" is a purchase receipt txo created by purchase_create. It is unclear there is any non-zero amount associated with a purchase receipt txo. Is this correct?

The intention of the original bug was to send only "other" funds, NOT purchase receipts (and NOT the claim types as you say). Transferring both "other" and "purchase" inside jsonrpc_account_fund() kind of makes sense because in that context both from/to accounts are in the same wallet.

I will work to exclude "purchase" txos from funds transferred by support_create, channel_create, stream_create, etc.

eukreign · 2022-07-11T13:38:14Z

lbry/extras/daemon/daemon.py

@@ -5470,11 +5509,11 @@ async def resolve(self, accounts, urls, **kwargs):
        return results

    @staticmethod
-    def _old_get_temp_claim_info(tx, txo, address, claim_dict, name, bid):
+    def _old_get_temp_claim_info(tx, txo, address, claim_dict, name):


Can you do this change as a separate PR? It seems somewhat unrelated to this one and this PR is already large and complicated.

Will do this next. I think the amount_to_dewies() and get_amount_or_error() stuff could also be unbundled. Then the function signatures and docstrings.

Pull request: #3635

eukreign · 2022-07-11T13:49:19Z

lbry/wallet/dewies.py

 from .util import coins_to_satoshis, satoshis_to_coins

+# Symbolic amount EVERYTHING
+AMOUNT_EVERYTHING = "EVERYTHING"


All caps variable already means this is a constant, doesn't need a comment above it saying the same.

eukreign · 2022-07-11T14:07:34Z

tests/integration/claims/test_claim_commands.py

+        await self.assertBalance(self.account, '0.0')
+        self.assertEqual(len(tx['outputs']), 1)  # no change
+        self.assertEqual(tx['outputs'][0]['amount'], '9.991457')
+        self.assertItemCount(await self.daemon.jsonrpc_channel_list(), 1)


Can you move this to the end of the test, this way you don't have to change the values of the other asserts.

…ludes purchase receipts.

moodyjon · 2022-07-28T01:50:42Z

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

eukreign · 2022-08-01T14:05:10Z

some of the unbundled PRs have been merged

lbry-bot assigned eukreign Apr 26, 2022

eukreign assigned moodyjon and unassigned eukreign Apr 27, 2022

moodyjon removed their assignment May 5, 2022

lbry-bot assigned eukreign May 5, 2022

eukreign assigned moodyjon and unassigned eukreign May 18, 2022

moodyjon force-pushed the send-max-pr1628 branch from 03d535d to 6a82b07 Compare May 18, 2022 17:15

moodyjon removed their assignment May 18, 2022

lbry-bot assigned eukreign May 18, 2022

moodyjon force-pushed the send-max-pr1628 branch 4 times, most recently from 06ef00e to dd0ed39 Compare May 26, 2022 17:55

eukreign requested changes Jun 6, 2022

View reviewed changes

tests/unit/wallet/test_transaction.py Outdated Show resolved Hide resolved

lbry-bot assigned eukreign and unassigned eukreign Jun 6, 2022

eukreign assigned moodyjon Jun 6, 2022

moodyjon added 10 commits June 9, 2022 11:51

Revising API away from decimal/str generalization. Use separate bool …

1fb3d83

…arg. (Derived from 33bdb50 but without test code.)

More revisions supporting bool args (bid_everything and amount_everyt…

6f12ea3

…hing). Make bid and amount args optional as well as args following them. Fold agrument checks into get_dewies_or_error().

Add some usages of bid_everything/amount_everything to test_claim_com…

42708fc

…mands.py.

Correct jsonrpc_account_fund handling of amount/everything.

0fce4f8

Correct some docstrings and regenerate api.json with scripts/generate…

c1fe4ca

…_json_api.py.

Update type annotations to reflect new args and behavior of get_dewie…

f7dfc66

…s_or_error().

Update expected error string on missing bid.

4a1b1da

Simplify InsufficientFundsError testcase.

07e8ab3

Convert --amount, --amount_everything to generic amount (int or str) …

cfefe99

…in daemon.py. Unpack the generic amount in transaction.py and account.py. Eliminate separate "everything" keyword args.

Use the definite txo.amount (not EVERYTHING) when calling save_claims().

8b2c284

moodyjon force-pushed the send-max-pr1628 branch from 9e9d511 to 8b2c284 Compare June 9, 2022 16:00

Use the definite txo.amount (not EVERYTHING) when calling save_suppor…

665c12b

…ts().

moodyjon requested a review from eukreign June 9, 2022 17:40

lbry-bot assigned eukreign and unassigned eukreign Jun 9, 2022

eukreign requested changes Jul 11, 2022

View reviewed changes

lbry-bot assigned eukreign and unassigned eukreign Jul 11, 2022

eukreign assigned moodyjon and unassigned eukreign Jul 11, 2022

lbry-bot assigned eukreign and unassigned moodyjon Jul 12, 2022

moodyjon mentioned this pull request Jul 18, 2022

Refactor _old_get_temp_claim_info(), eliminating "bid" arg. #3635

Merged

In the general case, limit utxo selection to txo_type=other which exc…

6cfcb6e

…ludes purchase receipts.

moodyjon marked this pull request as draft July 28, 2022 01:48

eukreign assigned moodyjon and unassigned eukreign Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

moodyjon commented Apr 26, 2022 •

edited

moodyjon commented Apr 26, 2022

eukreign commented Apr 27, 2022

moodyjon commented Apr 28, 2022

moodyjon commented May 5, 2022

moodyjon commented May 11, 2022

eukreign commented May 18, 2022

moodyjon commented May 18, 2022

moodyjon commented May 18, 2022

coveralls commented May 26, 2022 •

edited

moodyjon commented Jun 2, 2022

eukreign left a comment •

edited

moodyjon commented Jun 9, 2022

eukreign left a comment

eukreign Jul 11, 2022

eukreign Jul 11, 2022

eukreign Jul 11, 2022

moodyjon Jul 12, 2022

moodyjon Jul 22, 2022

eukreign Jul 11, 2022

moodyjon Jul 12, 2022

moodyjon Jul 22, 2022

eukreign Jul 11, 2022

eukreign Jul 11, 2022

moodyjon commented Jul 28, 2022

eukreign commented Aug 1, 2022

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

Are you sure you want to change the base?

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

Conversation

moodyjon commented Apr 26, 2022 • edited

moodyjon commented Apr 26, 2022

eukreign commented Apr 27, 2022

moodyjon commented Apr 28, 2022

moodyjon commented May 5, 2022

moodyjon commented May 11, 2022

eukreign commented May 18, 2022

moodyjon commented May 18, 2022

moodyjon commented May 18, 2022

coveralls commented May 26, 2022 • edited

moodyjon commented Jun 2, 2022

eukreign left a comment • edited

Choose a reason for hiding this comment

API

Internally

moodyjon commented Jun 9, 2022

eukreign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moodyjon commented Jul 28, 2022

eukreign commented Aug 1, 2022

moodyjon commented Apr 26, 2022 •

edited

coveralls commented May 26, 2022 •

edited

eukreign left a comment •

edited