Payment driver API #344

Wiezzel · 2020-06-15T16:02:45Z

Rationale

New API introduced for the communication between payment service and payment driver(s). It is meant to enable the implementation of the following requirements:

Supporting multiple accounts and payment drivers on a single yagna daemon.
Supporting multiple payment platforms (e.g. GNT, NGNT, NGNT-zkSync).
Allowing third-party developers to implement their own payment drivers with ease and possibly much freedom.

Detailed list of changes

BUS_ID is replaced with BUS_ID_PREFIX because each particular driver will have its own ID.

Currency enum is abandoned. It is not desired to have a fixed set of available currencies. It is assumed that all amounts and balances are given in currency appropriate for the platform.

All gas-related parameters are abandoned. They were too driver-specific to be included in the general API. It is recommended that payment drivers which use gas allow to configure limits (e.g. by env variables).

SchedulePayment does not include invoice_id parameter anymore. Instead, it returns an ID of the payment order generated by the driver.

GetPaymentStatus is removed because the new payment driver API relies on push notifications instead. That is realized by the NotifyPayment endpoint added to the payment service. It shall be called by a driver after every successful transaction.

Init message is removed from payment API (GSB). The init command from CLI will accept driver parameter and route the call directly to the appropriate driver.

RegisterAccount / UnregisterAccount messages are added because the payment service needs to maintain a list of available accounts. Payment drivers should call RegisterAccount after every successful initialization of an account to make it 'visible' to the payment service. UnregisterAccount should be called when the driver is being shut down. The list of available accounts shall not be persisted by the payment service. If an account doesn't need to be re-initialized after restarting yagna daemon, then payment driver should store this information and call RegisterAccount immediately after startup.

Wiezzel · 2020-06-15T16:08:18Z

Issue I failed to address in this API design: account permissions. Some kind of mechanism is necessary to control which identities are allowed to use which payment accounts. I couldn't figure out how and where to store this information appropriately. This however doesn't necessarily have to be done right now (we might just assume for the moment that every identity is allowed to use every payment account).

Wiezzel · 2020-06-15T20:51:13Z

Example flow:

Yagna daemon with two drivers starts
Simple GNT driver connects to GSB at local/driver/gnt
ZkSync driver connects to GSB at local/driver/gnt-zksync
User initializes their wallet 0xdeadbeef... in simple GNT driver via CLI
yagna payment init gnt -r 0xdeadbeef...
Init("0xdeadbeef...", R) message is sent to GNT driver via GSB
GSB driver sends RegisterAccount("gnt", "gnt", "0xdeadbeef...", R) to the payment service
User check their account balance via CLI
yagna payment status 0xdeadbeef...
Payment looks up in a hashmap that account 0xdeadbeef... is registered as handled by the GNT driver operating on GNT platform so it sends GetAccountBalance("gnt", "0xdeadbeef...") to the GNT driver.
Reported balance is displayed to the user.
User initializes their wallet 0xc0deface... in GNT-zkSync driver via CLI
yagna payment init gnt-zksync -rs 0xc0deface...
Init("0xc0deface...", R+S) message is sent to GNT-zkSync driver via GSB
GSB-zkSync driver sends RegisterAccount("gnt-zksync", "gnt-zksync", "0xc0deface...", R+S) to the payment service
Requestor agent acting as the user creates a demand for computation.
During demand creation market service asks payment service for available payment methods (we're probably going to need a new endpoint for that).
Payment service loops through all available accounts and concludes that only 0xc0deface... is in send mode so it is returned as the only result.
After the demand is matched an agreement is created specifying 0xc0deface as payer address and "gnt-zksync" as payment platform.
When the computation is done and the invoice gets accepted, payment service orders payment
Information about platform and account is retrieved from the market service (invoice has a relationship to the agreement).
Payment service looks up ("gnt-zksync", "0xc0deface...")in the hasmap and establishes that it is handled by the gnt-zksync driver so it sendsSchedulePaymentmessage with all information to the gnt-zksync driver. Driver responds with payment order ID"12345-abcdd-65421"`. That ID is stored by the payment service.
When the transaction is confirmed on blockchain, GNT-zkSync driver sends NotifyPayment message with all the details to the payment service.
Payment service forwards the payment details to the provider node.
On the provider node, another payment service verifies the transaction by looking up the appropriate driver for the payee account and sending VerifyPayment message.

Wiezzel · 2020-06-15T20:55:49Z

Other interesting scenario might be an integration with Ledger Live app that:

Uses Ledger hardware wallet for signing transactions instead of yagna identity.
Is initialized without using yagna CLI at all, just Ledger Live app starting, connecting to GSB and calling RegisterAccount.
Prompts the user to confirm the transaction when needed.

In general, I believe that integrating existing third-party wallets might be the most important use cases for the payment driver API (apart from our own driver, ofc). I tried to design the API in way that would allow this quite smoothly.

bidzyyys

I like the API 👍

core/model/src/driver.rs

bidzyyys · 2020-06-17T06:15:44Z

core/model/src/payment.rs

+
+    #[derive(Clone, Debug, Serialize, Deserialize)]
+    pub struct GetStatus{
+        pub platform: String,


Here it can't be done away with because we don't know the driver. address is not enough to establish that. There can be two different drivers handling e.g. GNT and ETH transactions but using the same account address.

core/model/src/payment.rs

bidzyyys · 2020-06-17T06:20:10Z

core/model/src/payment.rs

-        pub requestor: bool,
+    pub struct RegisterAccount {
+        pub driver: String,
+        pub platform: String,


Same here, both driver and platform are redundant.

IMHO only driver is necessary.

I believe the platform parameter has to stay here. Payment service has no other way to know which driver handles which platform.

bidzyyys · 2020-06-17T06:25:06Z

Issue I failed to address in this API design: account permissions. Some kind of mechanism is necessary to control which identities are allowed to use which payment accounts. I couldn't figure out how and where to store this information appropriately. This however doesn't necessarily have to be done right now (we might just assume for the moment that every identity is allowed to use every payment account).

@Wiezzel not exactly, think about the situation when somebody schedule payment as you and then you unlock the account -> driver sends transaction immediately. It is a possible attack.

bidzyyys · 2020-06-17T06:30:01Z

core/model/src/payment.rs

    }

    #[derive(Clone, Debug, Serialize, Deserialize)]
-    pub struct GetStatus(NodeId);
+    pub struct UnregisterAccount {
+        pub platform: String,


IMHO driver would be better and consistent with RegisterAccount

The mapping stored by payment service is (platform, address) -> (driver, mode). To remove an entry from a map we need to give the key as parameter. driver is not part of the key, while platform is.

Wiezzel · 2020-06-17T07:23:26Z

@Wiezzel not exactly, think about the situation when somebody schedule payment as you and then you unlock the account -> driver sends transaction immediately. It is a possible attack.

When I wrote "This however doesn't necessarily have to be done right now" I meant "We may ignore security for now". Obviously, not having permissions is insecure but we are on testnet now.

Rationale ---- New API introduced for the communication between payment service and payment driver(s). It is meant to enable the implementation of the following requirements: 1. Supporting multiple accounts and payment drivers on a single yagna daemon. 2. Supporting multiple payment platforms (e.g. GNT, NGNT, NGNT-zkSync). 3. Allowing third-party developers to implement their own payment drivers with ease and possibly much freedom. Detailed list of changes ---- `BUS_ID` is replaced with `BUS_ID_PREFIX` because each particular driver will have its own ID. Currency enum is abandoned. It is not desired to have a fixed set of available currencies. It is assumed that all amounts and balances are given in currency appropriate for the platform. All gas-related parameters are abandoned. They were too driver-specific to be included in the general API. It is recommended that payment drivers which use gas allow to configure limits (e.g. by env variables). `SchedulePayment` does not include `invoice_id` parameter anymore. Instead, it returns an ID of the payment order generated by the driver. `GetPaymentStatus` is removed because the new payment driver API relies on push notifications instead. That is realized by the `NotifyPayment` endpoint added to the payment service. It shall be called by a driver after every successful transaction. `Init` message is removed from payment API (GSB). The init command from CLI will accept `driver` parameter and route the call directly to the appropriate driver. `RegisterAccount` / `UnregisterAccount` messages are added because the payment service needs to maintain a list of available accounts. Payment drivers should call `RegisterAccount` after every successful initialization of an account to make it 'visible' to the payment service. `UnregisterAccount` should be called when the driver is being shut down. The list of available accounts shall not be persisted by the payment service. If an account doesn't need to be re-initialized after restarting yagna daemon, then payment driver should store this information and call `RegisterAccount` immediately after startup. Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

All-purpose PaymentError has been replaced with method-specific error types. All `unwrap()` calls have been replaced with `?` operator. Error message formatting has been moved to error module to avoid cluttering the processor code. Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Payment object represents a single (blockchain) transaction. It is possible that it includes activity payments and/or agreement payments from multiple users (but using the same wallet). Therefore, it can be also spending from multiple allocations. To adequately represent this situation `allocation_id` has been removed from Payment and added to ActivityPayment and AgreementPayment. Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

A new structure has been introducedFor the sake of preserving atomicity & consistency between `accounts` and `drivers` mappings . It allows to use a single lock for all operations. Also made some small adjustments in payment examples. Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Updated types to new api, removed get_payment_status

It needs to be used in non-static contexts as well. Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Cleaned up dummy driver create, added necessary calls to payment service (RegisterAccount and NotifyPayment). Adjusted payment_api example to work with the dummy driver service. Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Adjust payment service to new driver API

GNT Driver new API

Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Wiezzel requested review from maaktweluit and bidzyyys June 15, 2020 16:02

bidzyyys reviewed Jun 17, 2020

View reviewed changes

Wiezzel force-pushed the pay/driver-api branch 5 times, most recently from f410d19 to f95d1c2 Compare June 22, 2020 14:37

bidzyyys force-pushed the feature/gnt_driver_as_service branch 2 times, most recently from be729e5 to 77a1476 Compare June 24, 2020 06:46

Wiezzel force-pushed the pay/driver-api branch 2 times, most recently from 45f9b19 to 8e4dffc Compare July 3, 2020 11:29

Base automatically changed from feature/gnt_driver_as_service to master July 3, 2020 12:13

Wiezzel force-pushed the pay/driver-api branch from 8e4dffc to bd9dd72 Compare July 3, 2020 12:14

Wiezzel and others added 9 commits July 3, 2020 14:15

Adjusted payment service to new driver API

73794b6

Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Updated types to new api, removed get_payment_status

910c45e

Merge pull request #401 from golemfactory/mwu/driver-new-api

d1b7f92

Updated types to new api, removed get_payment_status

Adjust payment-dummy-driver to new api (#403)

0d2991c

Merge branch 'pay/driver-api' into wiezzel/payment-service

511de21

Changed driver_bus_id macro into function

8dae75d

It needs to be used in non-static contexts as well. Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Wiezzel and others added 13 commits July 9, 2020 16:43

Payment service + dummy driver working on new API

7fd4544

Cleaned up dummy driver create, added necessary calls to payment service (RegisterAccount and NotifyPayment). Adjusted payment_api example to work with the dummy driver service. Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Merge pull request #395 from golemfactory/wiezzel/payment-service

3518c9d

Adjust payment service to new driver API

Merge branch 'master' into pay/driver-api

0319378

chore: move GNT Driver to separate crate

737633e

chore: remove dead code

67dd749

feat: register account

35e3b31

feat: schedule payment generates order id

f6245c4

feat: notify payment

e59ce42

Adjusted payment_api example to work with the new GNT driver

fdbe851

docs: adjust instruction to new payment driver API

691f7d1

chore: apply fixes after CR

a3ab81e

feat: better error handling

d563db0

Merge pull request #415 from golemfactory/pay/gnt_new_api

149037a

GNT Driver new API

maaktweluit mentioned this pull request Jul 16, 2020

Changes required to run new payment api golemfactory/goth#180

Merged

Payment service: Do not send allocation IDs to provider

7f325f9

Signed-off-by: Adam Wierzbicki <awierzbicki@golem.network>

Wiezzel marked this pull request as ready for review July 16, 2020 13:49

bidzyyys self-requested a review July 16, 2020 14:43

bidzyyys approved these changes Jul 16, 2020

View reviewed changes

Wiezzel merged commit 8ecbe5f into master Jul 16, 2020

Wiezzel deleted the pay/driver-api branch July 16, 2020 14:44

maaktweluit mentioned this pull request Jul 17, 2020

Payments init and invoice payed golemfactory/goth#175

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Payment driver API #344

Payment driver API #344

Wiezzel commented Jun 15, 2020

Wiezzel commented Jun 15, 2020

Wiezzel commented Jun 15, 2020

Wiezzel commented Jun 15, 2020 •

edited

Loading

bidzyyys left a comment

bidzyyys Jun 17, 2020

Wiezzel Jun 17, 2020

bidzyyys Jun 17, 2020

bidzyyys Jun 17, 2020

Wiezzel Jun 17, 2020

bidzyyys commented Jun 17, 2020 •

edited

Loading

bidzyyys Jun 17, 2020 •

edited

Loading

Wiezzel Jun 17, 2020

Wiezzel commented Jun 17, 2020

Payment driver API #344

Payment driver API #344

Conversation

Wiezzel commented Jun 15, 2020

Rationale

Detailed list of changes

Wiezzel commented Jun 15, 2020

Wiezzel commented Jun 15, 2020

Wiezzel commented Jun 15, 2020 • edited Loading

bidzyyys left a comment

Choose a reason for hiding this comment

bidzyyys Jun 17, 2020

Choose a reason for hiding this comment

Wiezzel Jun 17, 2020

Choose a reason for hiding this comment

bidzyyys Jun 17, 2020

Choose a reason for hiding this comment

bidzyyys Jun 17, 2020

Choose a reason for hiding this comment

Wiezzel Jun 17, 2020

Choose a reason for hiding this comment

bidzyyys commented Jun 17, 2020 • edited Loading

bidzyyys Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Wiezzel Jun 17, 2020

Choose a reason for hiding this comment

Wiezzel commented Jun 17, 2020

Wiezzel commented Jun 15, 2020 •

edited

Loading

bidzyyys commented Jun 17, 2020 •

edited

Loading

bidzyyys Jun 17, 2020 •

edited

Loading