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

fix(background)!: better payment confirmation & message #694

Merged
merged 36 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c9ddb61
fix(background)!: better payment confirmation message
sidvishnoi Nov 1, 2024
05efcb0
wait before first attempt; rename function
sidvishnoi Nov 4, 2024
28d4b03
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 4, 2024
244ab38
make PaymentSession.pay always return OutgoingPayment; retry/throw ot…
sidvishnoi Nov 4, 2024
2148d3f
try different signature: get last outgoingPayment regardless of error
sidvishnoi Nov 4, 2024
a0ee3c6
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 7, 2024
30eb935
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 7, 2024
465ba9c
use async generator in polling; handle more errors; more abstraction
sidvishnoi Nov 7, 2024
951dc74
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 7, 2024
52abeaa
require only outgoingPaymentId in polling
sidvishnoi Nov 7, 2024
c96aa90
nit
sidvishnoi Nov 7, 2024
48cf703
nit: style/import
sidvishnoi Nov 7, 2024
6f93b1c
ui & message improvements
sidvishnoi Nov 8, 2024
d4e89fd
use same error message for `OutgoingPayment.failed = true`
sidvishnoi Nov 8, 2024
d57f918
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 11, 2024
4b13c73
reduce polling initial delay from 2.5s to 1.5s
sidvishnoi Nov 11, 2024
a0768ee
walletAddress check
sidvishnoi Nov 11, 2024
e11cf13
fix typo in error key name
sidvishnoi Nov 11, 2024
3df3acd
comment on null assertion
sidvishnoi Nov 11, 2024
cba0301
use payStatus full/partial instead of success/warn; also in bg
sidvishnoi Nov 11, 2024
d1e3681
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 13, 2024
bca0dbf
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 15, 2024
6560b41
update comment for isMissingGrantPermissionsError
sidvishnoi Nov 15, 2024
387cc08
handle case isMissingGrantPermissionsError is actually isTokenInactiv…
sidvishnoi Nov 15, 2024
715746e
update success/failure states; add partial status; update msgs
sidvishnoi Nov 15, 2024
2979f04
update success message to remove URL
sidvishnoi Nov 15, 2024
34ac427
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 19, 2024
b626ad3
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 22, 2024
3021b41
use opacity animation on message, slide is distracting
sidvishnoi Nov 22, 2024
3c8434c
fix disabled styles
sidvishnoi Nov 22, 2024
d2ae2ce
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 25, 2024
6efb7ba
update _locales key description
sidvishnoi Nov 25, 2024
9032cc4
handle insufficient grant by checking token's access values
sidvishnoi Nov 25, 2024
fb115d1
add todo comment
sidvishnoi Nov 25, 2024
0678549
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 26, 2024
5347047
Merge branch 'main' into payment-confirmation
sidvishnoi Nov 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/_locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,37 @@
"keyRevoked_action_reconnectBtn": {
"message": "Reconnect"
},
"pay_action_pay": {
"message": "Send now"
},
"pay_error_notEnoughFunds": {
"message": "Insufficient funds to complete the payment."
},
"pay_error_outgoingPaymentFailed": {
"message": "We were unable to process this transaction. Please try again!",
"description": "OutgoingPayment.failed === true"
Copy link
Member

Choose a reason for hiding this comment

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

Failed should happen when outgoing_payment.failed = false and outgoing_payment.sent_amount === 0, since the OP can fail during sending and only send a partial amount.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be clear, OutgoingPayment.failed = true and OutgoingPayment.sent_amount > 0 is possible?

We want to treat any partial amount to be treated as success here?

Copy link
Member

@raducristianpopa raducristianpopa Nov 11, 2024

Choose a reason for hiding this comment

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

Just to be clear, OutgoingPayment.failed = true and OutgoingPayment.sent_amount > 0 is possible?

Yes.

We want to treat any partial amount to be treated as success here?

We cannot treat a sent partial amount as an error (failed = true && sent_amount = 0), since an amount was actually sent. We can use payStatus.type = 'partial' for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed cases with @raducristianpopa on Slack. We need to handle these cases:

failed = T; sent_amount = 0             => definitely error. immediate fail.
failed = F; sent_amount = 0             => we don't know yet. maybe warn user? It could be an error if check for long enough, or sent_amount might become non-zero

failed = T; sent_amount > 0             => ok, but only a partial was sent
failed = F; sent_amount > 0             => ok, but only a partial was sent, can be successfully completed afterwards

failed = T; sent_amount = debit_amount  => not possible
failed = F; sent_amount = debit_amount  => ok. immediate success.

@RabebOthmani How to best show this to user? For added context, after the payment, we've to repeatedly check payment status (polling). Right now, we check for up to 8 seconds only. The payment might be in any of above 5 states.

Also, note that, we'll be showing user a spinner for those 8s. Might be better to show "verifying payment status" message while we are in verify stage?

Choose a reason for hiding this comment

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

8s is a long time for just a spinner. My unsolicited opinion is that we should show some kind of messaging for sure.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Per Slack:

  • " It looks like this is taking long. Check your wallet later to ensure the payment went through"
  • Partial payment- only was successfully sent. to learn more

},
"pay_warn_pay_warn_outgoingPaymentPollingIncomplete": {
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
"message": "We could not verify if the payment was completed. Please ensure you've enough funds in your wallet.",
"description": "We were polling for completion, but it's not finished yet - could mean it'll never succeed or it'll take a long time to settle."
},
"pay_error_general": {
"message": "We were unable to process this transaction. Please try again!"
},
"pay_error_invalidReceivers": {
"message": "At the moment, you cannot pay this website.",
"description": "We cannot send money (probable cause: un-peered wallets)"
},
"pay_error_notMonetized": {
"message": "This website is not monetized."
},
"pay_state_success": {
"message": "Thanks for your support! $AMOUNT$ was sent to $WEBSITE_URL$.",
"placeholders": {
"AMOUNT": { "content": "$1", "example": "$2.05" },
"WEBSITE_URL": { "content": "$2", "example": "https://example.com" }
}
},
"outOfFunds_error_title": {
"message": "Out of funds"
},
Expand Down
5 changes: 5 additions & 0 deletions src/background/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,8 @@ export const MAX_RATE_OF_PAY = '100';

export const EXCHANGE_RATES_URL =
'https://telemetry-exchange-rates.s3.amazonaws.com/exchange-rates-usd.json';

export const OUTGOING_PAYMENT_POLLING_MAX_DURATION = 8_000;
export const OUTGOING_PAYMENT_POLLING_INITIAL_DELAY = 2500;
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
export const OUTGOING_PAYMENT_POLLING_INTERVAL = 1500;
export const OUTGOING_PAYMENT_POLLING_MAX_ATTEMPTS = 8;
2 changes: 1 addition & 1 deletion src/background/services/background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ export class Background {

case 'PAY_WEBSITE':
return success(
await this.monetizationService.pay(message.payload.amount),
await this.monetizationService.pay(message.payload),
);

// endregion
Expand Down
109 changes: 97 additions & 12 deletions src/background/services/monetization.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,31 @@
import type { Runtime, Tabs } from 'webextension-polyfill';
import {
import type {
PayWebsitePayload,
PayWebsiteResponse,
ResumeMonetizationPayload,
StartMonetizationPayload,
StopMonetizationPayload,
} from '@/shared/messages';
import { PaymentSession } from './paymentSession';
import { computeRate, getSender, getTabId } from '../utils';
import { isOutOfBalanceError } from './openPayments';
import { isOkState, removeQueryParams } from '@/shared/helpers';
import {
isMissingGrantPermissionsError,
isOutOfBalanceError,
} from './openPayments';
import {
OUTGOING_PAYMENT_POLLING_MAX_ATTEMPTS,
OUTGOING_PAYMENT_POLLING_MAX_DURATION,
} from '@/background/config';
import {
ErrorWithKey,
formatCurrency,
isErrorWithKey,
isOkState,
removeQueryParams,
} from '@/shared/helpers';
import { transformBalance } from '@/popup/lib/utils';
import type { AmountValue, PopupStore, Storage } from '@/shared/types';
import type { OutgoingPayment } from '@interledger/open-payments';
import type { Cradle } from '../container';

export class MonetizationService {
Expand Down Expand Up @@ -249,7 +266,7 @@ export class MonetizationService {
}
}

async pay(amount: string) {
async pay({ amount }: PayWebsitePayload): Promise<PayWebsiteResponse> {
const tab = await this.windowState.getCurrentTab();
if (!tab || !tab.id) {
throw new Error('Unexpected error: could not find active tab.');
Expand All @@ -263,27 +280,95 @@ export class MonetizationService {
throw new Error(this.t('pay_error_notMonetized'));
}

const { walletAddress } = await this.storage.get(['walletAddress']);
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
const { url: tabUrl } = this.tabState.getPopupTabData(tab);

const splitAmount = Number(amount) / payableSessions.length;
// TODO: handle paying across two grants (when one grant doesn't have enough funds)
const results = await Promise.allSettled(
payableSessions.map((session) => session.pay(splitAmount)),
);

const totalSentAmount = results
.filter((e) => e.status === 'fulfilled')
.reduce(
(acc, curr) => acc + BigInt(curr.value?.debitAmount.value ?? 0),
0n,
);
const outgoingPayments = new Map<string, OutgoingPayment | null>(
payableSessions.map((s, i) => [
s.id,
results[i].status === 'fulfilled' ? results[i].value : null,
]),
);
this.logger.debug('polling outgoing payments for completion');
const signal = AbortSignal.timeout(OUTGOING_PAYMENT_POLLING_MAX_DURATION); // can use other signals as well, such as popup closed etc.
const pollingResults = await Promise.allSettled(
[...outgoingPayments]
.filter(([, outgoingPayment]) => outgoingPayment !== null)
.map(async ([sessionId, outgoingPaymentInitial]) => {
for await (const outgoingPayment of this.openPaymentsService.pollOutgoingPayment(
outgoingPaymentInitial!.id,
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
{ signal, maxAttempts: OUTGOING_PAYMENT_POLLING_MAX_ATTEMPTS },
)) {
outgoingPayments.set(sessionId, outgoingPayment);
}
return outgoingPayments.get(sessionId)!;
}),
);

const totalSentAmount = [...outgoingPayments.values()].reduce(
(acc, op) => acc + BigInt(op?.sentAmount?.value ?? 0),
raducristianpopa marked this conversation as resolved.
Show resolved Hide resolved
0n,
);
if (totalSentAmount === 0n) {
const pollingErrors = pollingResults
.filter((e) => e.status === 'rejected')
.map((e) => e.reason);

if (pollingErrors.some((e) => isMissingGrantPermissionsError(e))) {
// This permission request to read outgoing payments was added at a
// later time, so existing connected wallets won't have this permission.
// Assume as success for backward compatibility.
const totalDebitAmount = [...outgoingPayments.values()].reduce(
(acc, op) => acc + BigInt(op?.debitAmount?.value ?? 0),
0n,
);
const { assetScale, assetCode } = walletAddress!;
const sentAmount = transformBalance(totalDebitAmount, assetScale);
return {
type: 'success',
sentAmount: sentAmount,
sentAmountFormatted: formatCurrency(sentAmount, assetCode),
url: tabUrl,
};
}

const isNotEnoughFunds = results
.filter((e) => e.status === 'rejected')
.some((e) => isOutOfBalanceError(e.reason));
const isPollingLimitReached = pollingErrors.some(
(err) =>
(isErrorWithKey(err) &&
err.key === 'pay_warn_pay_warn_outgoingPaymentPollingIncomplete') ||
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
(err instanceof DOMException && err.name === 'TimeoutError'),
);

if (isNotEnoughFunds) {
throw new Error(this.t('pay_error_notEnoughFunds'));
throw new ErrorWithKey('pay_error_notEnoughFunds');
}
throw new Error('Could not facilitate payment for current website.');
if (isPollingLimitReached) {
throw new ErrorWithKey(
'pay_warn_pay_warn_outgoingPaymentPollingIncomplete',
);
}
throw new ErrorWithKey('pay_error_general');
}

// TODO: If sentAmount is non-zero but less than to debitAmount, show
// warning that not entire payment went through (yet?)
const { assetCode, assetScale } = walletAddress!;
const sentAmount = transformBalance(totalSentAmount, assetScale);
return {
type: 'success',
sentAmount: sentAmount,
sentAmountFormatted: formatCurrency(sentAmount, assetCode),
url: tabUrl,
};
}

private canTryPayment(
Expand Down
58 changes: 57 additions & 1 deletion src/background/services/openPayments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
errorWithKeyToJSON,
getWalletInformation,
isErrorWithKey,
sleep,
withResolvers,
type ErrorWithKeyLike,
} from '@/shared/helpers';
Expand All @@ -44,6 +45,8 @@ import {
DEFAULT_RATE_OF_PAY,
MAX_RATE_OF_PAY,
MIN_RATE_OF_PAY,
OUTGOING_PAYMENT_POLLING_INTERVAL,
OUTGOING_PAYMENT_POLLING_INITIAL_DELAY,
} from '../config';
import { OPEN_PAYMENTS_REDIRECT_URL } from '@/shared/defines';
import type { Cradle } from '@/background/container';
Expand Down Expand Up @@ -694,7 +697,7 @@ export class OpenPaymentsService {
},
{
type: 'outgoing-payment',
actions: ['create'],
actions: ['create', 'read'],
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
identifier: walletAddress.id,
limits: {
debitAmount: {
Expand Down Expand Up @@ -881,6 +884,50 @@ export class OpenPaymentsService {
return outgoingPayment;
}

/** Polls for the completion of an outgoing payment */
async *pollOutgoingPayment(
outgoingPaymentId: OutgoingPayment['id'],
{
signal,
maxAttempts = 10,
}: Partial<{ signal: AbortSignal; maxAttempts: number }> = {},
): AsyncGenerator<OutgoingPayment, OutgoingPayment, void> {
let attempt = 0;
await sleep(OUTGOING_PAYMENT_POLLING_INITIAL_DELAY);
while (++attempt <= maxAttempts) {
try {
signal?.throwIfAborted();
const outgoingPayment = await this.client!.outgoingPayment.get({
url: outgoingPaymentId,
accessToken: this.token.value,
});
yield outgoingPayment;
if (outgoingPayment.failed) {
throw new ErrorWithKey('pay_error_outgoingPaymentFailed');
}
if (
outgoingPayment.debitAmount.value === outgoingPayment.sentAmount.value
) {
return outgoingPayment; // completed
}
signal?.throwIfAborted();
await sleep(OUTGOING_PAYMENT_POLLING_INTERVAL);
} catch (error) {
if (isMissingGrantPermissionsError(error)) {
throw error;
} else if (isTokenExpiredError(error)) {
await this.rotateToken();
} else {
throw error;
}
}
}

throw new ErrorWithKey(
'pay_warn_pay_warn_outgoingPaymentPollingIncomplete',
);
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
}

async probeDebitAmount(
amount: AmountValue,
incomingPayment: IncomingPayment['id'],
Expand Down Expand Up @@ -1023,6 +1070,15 @@ export const isOutOfBalanceError = (error: any) => {
return error.status === 403 && error.description === 'unauthorized';
};

export const isMissingGrantPermissionsError = (error: any) => {
if (!isOpenPaymentsClientError(error)) return false;
return (
error.status === 403 &&
(error.description === 'Insufficient Grant' /* Fynbos */ ||
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
error.description === 'Inactive Token')
sidvishnoi marked this conversation as resolved.
Show resolved Hide resolved
);
};

export const isInvalidReceiverError = (error: any) => {
if (!isOpenPaymentsClientError(error)) return false;
return error.status === 400 && error.description === 'invalid receiver';
Expand Down
3 changes: 3 additions & 0 deletions src/background/services/paymentSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { transformBalance } from '@/popup/lib/utils';
import {
isInvalidReceiverError,
isKeyRevokedError,
isMissingGrantPermissionsError,
isNonPositiveAmountError,
isOutOfBalanceError,
isTokenExpiredError,
Expand Down Expand Up @@ -410,6 +411,8 @@ export class PaymentSession {
if (isKeyRevokedError(e)) {
this.events.emit('open_payments.key_revoked');
throw e;
} else if (isMissingGrantPermissionsError(e)) {
throw e;
} else if (isTokenExpiredError(e)) {
await this.openPaymentsService.rotateToken();
return await this.pay(amount); // retry
Expand Down
Loading