Skip to content
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

wallet servers requesting a daily fee will automatically get paid by client #2707

Merged
merged 10 commits into from
Feb 18, 2020

Conversation

shyba
Copy link
Member

@shyba shyba commented Jan 6, 2020

continuation of #2683

Changes in this PR:

New SDK component: WALLET_SERVER_PAYMENTS_COMPONENT

New configuration key: max_wallet_server_fee (defaults to 1.0)

Wallet server config options: PAYMENT_ADDRESS and DAILY_FEE

During SDK startup, if its not set to skip, the component for payments starts by reading max_wallet_server_fee then sleeping for 24 hours.

On the wallet server, operators can set the configurations for a daily fee and the address for it, however there isn't any enforcement yet.

After 24 hours, the component wakes up, pays to the current "master" server and goes back to sleep. This only happens if there is a valid address and fee set on the main server and that fee is below the configured threshold.

It also sends analytics when after it pays

@shyba shyba mentioned this pull request Jan 6, 2020
@shyba shyba force-pushed the pay_wallet_servers branch 4 times, most recently from 4be45c6 to 32614d2 Compare January 13, 2020 15:34
@shyba shyba changed the title wip: Pay wallet servers Pay wallet servers Jan 13, 2020
@shyba shyba force-pushed the pay_wallet_servers branch 3 times, most recently from 4a51d05 to 7da1f42 Compare January 20, 2020 05:14
@shyba shyba requested a review from eukreign February 5, 2020 01:25
@lbry-bot lbry-bot assigned eukreign and unassigned shyba Feb 5, 2020
Copy link
Member

@eukreign eukreign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it the intention to drop support for sending donations and only allow required payments? Personally I think it's reasonable to leave the existing donation feature and just add the required payment feature next to it.

@@ -79,11 +81,14 @@ def accounts(self):
await asyncio.gather(*(
l.start() for l in self.ledgers.values()
))
self.usage_payment_service = WalletServerPayer(self.ledger, self.default_wallet)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be instantiated in __init__ then you won't need to add Optional to the type?

LEDGER = lbry.wallet

def setUp(self) -> None:
WalletServerPayer.PAYMENT_PERIOD = 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this just be a configurable thing in the WalletServerPayer (eg. an argument to WalletServerPayer.init which then uses the PAYMENT_PERIOD constant as a default)?



class TestUsagePayment(CommandTestCase):
LEDGER = lbry.wallet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer necessary.

tx = await Transaction.create([],
[Output.pay_pubkey_hash(amount, self.ledger.address_to_hash160(address))],
self.wallet.get_accounts_or_all(None),
self.wallet.get_account_or_default(None))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting is strange, needs indentation fix

self.wallet.get_account_or_default(None))

await self.ledger.broadcast(tx)
# await self.analytics_manager.send_credits_sent() fixme: handle that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this difficult to handle?

if not self.ledger.is_valid_address(address):
raise Exception(f"Invalid address: {address}")
if self.wallet.is_locked:
raise Exception("Cannot spend funds with locked wallet")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will kill the pay service, even after the wallet is unlocked no payment will be sent. Perhaps just print a message and continue running?

@lbry-bot lbry-bot assigned shyba and unassigned eukreign Feb 9, 2020
@shyba shyba force-pushed the pay_wallet_servers branch 3 times, most recently from b9e0c48 to b261df0 Compare February 12, 2020 01:20
@shyba shyba requested a review from eukreign February 14, 2020 16:06
@lbry-bot lbry-bot assigned eukreign and unassigned shyba Feb 14, 2020
@lbry-bot lbry-bot assigned shyba and unassigned eukreign Feb 17, 2020
@eukreign eukreign changed the title Pay wallet servers wallet servers requiring a daily fee will get paid by client Feb 17, 2020
@eukreign eukreign changed the title wallet servers requiring a daily fee will get paid by client wallet servers requiring a daily fee will automatically get paid by client Feb 17, 2020
@eukreign eukreign added area: hub type: new feature New functionality that does not exist yet labels Feb 17, 2020
@eukreign eukreign merged commit 4b87cb4 into master Feb 18, 2020
@eukreign eukreign deleted the pay_wallet_servers branch February 18, 2020 22:40
@lyoshenka lyoshenka changed the title wallet servers requiring a daily fee will automatically get paid by client wallet servers requesting a daily fee will automatically get paid by client Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: hub type: new feature New functionality that does not exist yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants