Include existing claim amount in the max biddable amount for a publish #1108

hackrush01 · 2018-02-09T02:05:39Z

Fixes updating a claim when update's bid amount is greater than wallet balance but it can use the claim's original amount to complete the transaction.

hackrush01 · 2018-02-09T02:08:42Z

Forgot changelog.

jackrobison

Instead of removing the balance checks you should update them to use fresh numbers from lbryum. lbrynet.core.Wallet.update_balance probably is what you're looking for

jackrobison · 2018-02-12T19:23:29Z

lbrynet/daemon/Daemon.py

@@ -715,8 +715,6 @@ def _publish_stream(self, name, bid, claim_dict, file_path=None, certificate_id=
        publisher = Publisher(self.session, self.lbry_file_manager, self.session.wallet,
                              certificate_id)
        parse_lbry_uri(name)
-        if bid <= 0.0:


The reason this is here is because otherwise we have to possibly make a stream before lbryum would tell us something is wrong

jackrobison · 2018-02-12T19:23:46Z

lbrynet/daemon/Daemon.py

@@ -1969,10 +1967,6 @@ def jsonrpc_publish(self, name, bid, metadata=None, file_path=None, fee=None, ti
        if bid <= 0.0:
            raise Exception("Invalid bid")

-        if bid >= self.session.wallet.get_balance():
-            raise InsufficientFundsError('Insufficient funds. ' \


Same reason as the other bid check

Also, this should be formatted as something like:

raise InsufficientFundsError( 'Insufficient funds. Make sure you have enough LBC to deposit.' )

Or perhaps, if the message is constant it should just be built-in to the InsufficientFundsError. How many different messages do we need for this exception?

eukreign · 2018-02-12T20:05:32Z

lbrynet/daemon/Daemon.py

@@ -1969,10 +1967,6 @@ def jsonrpc_publish(self, name, bid, metadata=None, file_path=None, fee=None, ti
        if bid <= 0.0:
            raise Exception("Invalid bid")

-        if bid >= self.session.wallet.get_balance():
-            raise InsufficientFundsError('Insufficient funds. ' \


Also, this should be formatted as something like:

raise InsufficientFundsError( 'Insufficient funds. Make sure you have enough LBC to deposit.' )

Or perhaps, if the message is constant it should just be built-in to the InsufficientFundsError. How many different messages do we need for this exception?

eukreign · 2018-02-12T20:06:40Z

lbrynet/tests/unit/core/test_Wallet.py

@@ -217,7 +217,7 @@ def update_balance():
        d.addCallback(lambda _: self.assertEqual(5, wallet.get_balance()))
        d.addCallback(lambda _: wallet.reserve_points('testid', 2))
        d.addCallback(lambda _: wallet.claim_name('test', 4, test_claim_dict))
-        self.assertFailure(d, InsufficientFundsError)
+        self.assertFailure(d, Exception)


Testing for Exception seems a bit dangerous. This would pass the test on any exception, even if it has nothing to do with the error being tested.

This is what is returned from lbryum I guess, maybe I need to look at the lbryum error messages too.
Also would it be bad practice if I check the error message string and then decide which exception to raise?? Something like

if claim["msg"] == "Insufficient funds": Raise InsufficientFundsError()

here ^ claim is the JSON signature returned from lbryum

Checking for an error condition in a response from lbryum and throwing an appropriate exception makes sense to me. Although lbryum is not consistent about the insufficient funds errors, maybe we need to add error codes first as @kauffj suggested in another issue.

Yes, the function for checking the above stuff in lbryum is a bit convoluted, with many abstract returns. Maybe I'll check for error message in daemon and raise specific Exceptions for possible cases, and a different but non-generic and descriptive errors for the rest of the cases.

hackrush01 · 2018-02-13T13:56:01Z

@jackrobison
lbrynet.core.Wallet.update_balance only gets the updated wallet balance, but the issue I'm trying to fix is this #748.

For e.g. I have:
10 LBC in wallet
Claim qwerty with bid 20 LBC
I should be able to update qwerty's bid to 25 LBC, which would require spending the claimtrietx input coin. This already has support in lbryum, just removing the checks from the daemon for it.

jackrobison · 2018-02-13T15:01:34Z

Perhaps you could add a function to the lbryum wallet to return the maximum spendable amount for an update. Something like:

def get_maximum_claim_amount(self, claim_id=None):
    return confirmed_balance + current_claim_amount or 0

Then have lbrynet publish check that the bid is less than this amount instead of the balance.

eukreign

A few minor things. Otherwise, LGTM!

eukreign · 2018-02-20T02:44:08Z

lbrynet/core/Wallet.py

+    @defer.inlineCallbacks
+    def get_max_usable_balance_for_claim(self, claim_name):
+        amt = yield self._run_cmd_as_defer_to_thread('get_max_spendable_amt_for_claim', claim_name)
+        defer.returnValue(amt)


You can simplify this to just:

def get_max_usable_balance_for_claim(self, claim_name): return self._run_cmd_as_defer_to_thread('get_max_spendable_amt_for_claim', claim_name)

@defer.inlineCallbacks turns a method into something that returns a Deferred but _run_cmd_as_defer_to_thread() already returns a Deferred, so you can skip all the indirection and just return the result of _run_cmd_as_defer_to_thread().

eukreign · 2018-02-20T02:46:30Z

lbrynet/daemon/Daemon.py

+        amt = yield self.session.wallet.get_max_usable_balance_for_claim(name)
+        if bid > amt:
+            raise InsufficientFundsError(
+                "Please lower the bid value, the max amount you can specify for this claim is {}"


In user facing messages we should spell out 'max' as maximum. If this text doesn't make it to the user then never mind.

Fixes #748 Fixed failing test, Yay! Why CHANGELOG? Why not anarchy?

jackrobison · 2018-02-20T19:11:31Z

CHANGELOG.md

@@ -26,7 +26,8 @@ at anytime.
  * `get` failing with a non-useful error message when given a uri for a channel claim
  * exception checking in several wallet unit tests
  * daemon not erring properly for non-numeric values being passed to the `bid` parameter for the `publish` method
-  *
+  * publish command to allow updating claims with bid amount higher than wallet balance(by spending the claimtrietx coin) (https://github.com/lbryio/lbry/issues/748)


Commands, classes, functions, and argument names should be decorated with code ( `) tags. In this entry, publish and bid should be decorated.

higher than wallet balance

How much higher? Could be rephrased: with a bid amount higher than the wallet balance, so long as the amount is less than the wallet balance plus the bid amount of the claim being updated

jackrobison · 2018-02-20T19:18:29Z

lbrynet/core/Wallet.py

@@ -975,6 +976,9 @@ def _update_balance(self):
            lambda result: Decimal(result['confirmed']) + Decimal(result.get('unconfirmed', 0.0)))
        return d

+    def get_max_usable_balance_for_claim(self, claim_name):


Please add a matching dummy method in Wallet too

hackrush01 · 2018-02-21T08:55:14Z

@jackrobison I made the required changes. Please check.

hackrush01 self-assigned this Feb 9, 2018

hackrush01 requested a review from jackrobison February 9, 2018 02:05

lbry-bot assigned jackrobison and unassigned hackrush01 Feb 9, 2018

lyoshenka requested review from eukreign and removed request for jackrobison February 9, 2018 15:09

lbry-bot assigned eukreign and unassigned jackrobison Feb 9, 2018

jackrobison requested changes Feb 12, 2018

View reviewed changes

eukreign requested changes Feb 12, 2018

View reviewed changes

lbry-bot assigned hackrush01 and unassigned eukreign and hackrush01 Feb 12, 2018

lbry-bot assigned hackrush01 and unassigned hackrush01 Feb 13, 2018

hackrush01 force-pushed the update_fix branch 3 times, most recently from 27b4d0a to a50d6a1 Compare February 14, 2018 19:48

hackrush01 requested review from eukreign and jackrobison February 14, 2018 19:50

lbry-bot assigned eukreign and jackrobison and unassigned hackrush01 Feb 14, 2018

hackrush01 mentioned this pull request Feb 18, 2018

finish and merge PR for #748 #1082

Closed

eukreign requested changes Feb 20, 2018

View reviewed changes

lbry-bot unassigned eukreign Feb 20, 2018

hackrush01 added 2 commits February 20, 2018 08:54

Removed daemon side bid checks during publish

d72ef78

Fixes #748 Fixed failing test, Yay! Why CHANGELOG? Why not anarchy?

Check for max usable balance before updating

2368433

hackrush01 force-pushed the update_fix branch 3 times, most recently from 3559d28 to 99dda5f Compare February 20, 2018 04:11

jackrobison requested changes Feb 20, 2018

View reviewed changes

lbry-bot assigned hackrush01 and unassigned jackrobison Feb 20, 2018

jackrobison changed the title ~~Update fix~~ Include existing claim amount in the max biddable amount for a publish Feb 20, 2018

Review fixes and additional comments in test

3754f34

hackrush01 force-pushed the update_fix branch from 99dda5f to 3754f34 Compare February 21, 2018 08:52

lbry-bot assigned hackrush01 and unassigned hackrush01 Feb 21, 2018

jackrobison approved these changes Feb 22, 2018

View reviewed changes

Merge branch 'master' into update_fix

dafa80c

jackrobison merged commit dafa80c into master Feb 22, 2018

lyoshenka deleted the update_fix branch February 22, 2018 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include existing claim amount in the max biddable amount for a publish #1108

Include existing claim amount in the max biddable amount for a publish #1108

hackrush01 commented Feb 9, 2018 •

edited

hackrush01 commented Feb 9, 2018

jackrobison left a comment

jackrobison Feb 12, 2018

jackrobison Feb 12, 2018

eukreign Feb 12, 2018

eukreign Feb 12, 2018

eukreign Feb 12, 2018

hackrush01 Feb 13, 2018 •

edited

eukreign Feb 13, 2018

hackrush01 Feb 13, 2018

hackrush01 commented Feb 13, 2018

jackrobison commented Feb 13, 2018

eukreign left a comment

eukreign Feb 20, 2018

eukreign Feb 20, 2018

jackrobison Feb 20, 2018

jackrobison Feb 20, 2018

jackrobison Feb 20, 2018

hackrush01 Feb 21, 2018

hackrush01 commented Feb 21, 2018

Include existing claim amount in the max biddable amount for a publish #1108

Include existing claim amount in the max biddable amount for a publish #1108

Conversation

hackrush01 commented Feb 9, 2018 • edited

hackrush01 commented Feb 9, 2018

jackrobison 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

hackrush01 Feb 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackrush01 commented Feb 13, 2018

jackrobison commented Feb 13, 2018

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

hackrush01 commented Feb 21, 2018

hackrush01 commented Feb 9, 2018 •

edited

hackrush01 Feb 13, 2018 •

edited