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

feat(processor): any-declarative payment in NEAR #1428

Merged
merged 3 commits into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 2 additions & 3 deletions packages/payment-processor/src/payment/any-to-erc20-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
import { padAmountForChainlink } from '@requestnetwork/payment-detection';
import { IPreparedTransaction } from './prepared-transaction';
import { IConversionPaymentSettings } from './index';
import { validatePaymentReference } from '../utils/validation';

/**
* Processes a transaction to pay a request with an ERC20 currency that is different from the request currency (eg. fiat).
Expand Down Expand Up @@ -153,9 +154,7 @@ function prepareAnyToErc20Arguments(

const { paymentReference, paymentAddress, feeAddress, feeAmount, maxRateTimespan } =
getRequestPaymentValues(request);
if (!paymentReference) {
throw new Error('paymentReference is missing');
}
validatePaymentReference(paymentReference);
const amountToPay = padAmountForChainlink(getAmountToPay(request, amount), requestCurrency);
const feeToPay = padAmountForChainlink(feeAmountOverride || feeAmount || 0, requestCurrency);
return {
Expand Down
40 changes: 40 additions & 0 deletions packages/payment-processor/src/payment/near-amount-with-ref.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { BigNumberish } from 'ethers';
import { WalletConnection } from 'near-api-js';

import { CurrencyTypes } from '@requestnetwork/types';

import { INearTransactionCallback, processNearPayment } from './utils-near';
import { NearChains } from '@requestnetwork/currency';

/**
* Processes a transaction to make a payment in NEAR token with a reference.
*
* @notice This is used to pay a declarative request, with low-level arguments.
*
* @param paymentAddress must be a valid NEAR address on the given network.
* @param network e.g. 'near'
* @param paymentReference used to index payments.
* @param walletConnection the Near provider.
* @param amount amount to pay, in NEAR tokens.
*/
// FIXME: We could improve the method's interface by enforcing a type on `paymentInfo` for declarative requests.
Comment on lines +9 to +20
Copy link

Choose a reason for hiding this comment

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

Documentation is clear and helpful.

The function documentation provides a clear description of the function's purpose and parameters.

Offer assistance for FIXME comment.

The FIXME comment suggests improving the method's interface by enforcing a type on paymentInfo.

Would you like me to help enforce a type on paymentInfo for declarative requests?

export async function payNearAmountWithReference(
paymentAddress: string,
paymentReference: string,
network: CurrencyTypes.NearChainName,
walletConnection: WalletConnection,
amount: BigNumberish,
callback?: INearTransactionCallback,
): Promise<void> {
NearChains.assertChainSupported(network);

return processNearPayment(
walletConnection,
network,
amount,
paymentAddress,
paymentReference,
'0.2.0',
callback,
);
}
13 changes: 4 additions & 9 deletions packages/payment-processor/src/payment/near-conversion.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@ import {
import { INearTransactionCallback, processNearPaymentWithConversion } from './utils-near';
import { IConversionPaymentSettings } from '.';
import { CurrencyManager, NearChains, UnsupportedCurrencyError } from '@requestnetwork/currency';
import { validatePaymentReference } from '../utils/validation';

/**
* Processes the transaction to pay a request in NEAR with on-chain conversion.
* @param request the request to pay
* @param walletConnection the Web3 provider, or signer. Defaults to window.ethereum.
* @param amount optionally, the amount to pay. Defaults to remaining amount of the request.
* @param walletConnection the Near provider.
* @param amount optionally, the amount to pay in request currency. Defaults to remaining amount of the request.
Comment on lines +20 to +21
Copy link

@coderabbitai coderabbitai bot Jul 15, 2024

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Ensure mockedNearWalletConnection conforms to NearProvider type

The mockedNearWalletConnection in the test file packages/payment-processor/test/payment/any-to-near.test.ts is currently cast to any, which bypasses type checking. To ensure type safety and correctness, it should be explicitly typed to conform to the NearProvider type.

  • Update the definition of mockedNearWalletConnection to explicitly use the NearProvider type.
Analysis chain

Verify the usage of NearProvider type for walletConnection.

Ensure that all instances where payNearConversionRequest is called have been updated to use NearProvider for the walletConnection parameter.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `payNearConversionRequest` use `NearProvider` for the `walletConnection` parameter.

# Test: Search for the function usage. Expect: Only occurances of the new signature.
rg --type typescript -A 5 $'payNearConversionRequest'

Length of output: 87


Script:

#!/bin/bash
# Description: Verify all function calls to `payNearConversionRequest` use `NearProvider` for the `walletConnection` parameter.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type ts -A 5 $'payNearConversionRequest'

Length of output: 3780


Script:

#!/bin/bash
# Description: Verify the type of `mockedNearWalletConnection` used in `payNearConversionRequest` calls.

# Test: Search for the definition or initialization of `mockedNearWalletConnection`.
rg 'mockedNearWalletConnection' packages/payment-processor/test/payment/any-to-near.test.ts

Length of output: 577


Script:

#!/bin/bash
# Description: Verify the type of `mockedNearWalletConnection` used in `payNearConversionRequest` calls by inspecting its full definition.

# Test: Search for the complete definition of `mockedNearWalletConnection` and capture context lines.
rg 'const mockedNearWalletConnection' -A 10 packages/payment-processor/test/payment/any-to-near.test.ts

Length of output: 1468

Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a correct observation - the typing in the test could use NearProvider instead of any 👍

Copy link

Choose a reason for hiding this comment

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

@MantisClone, great! I'm glad you agree with the observation. I'll make a note of this preference for future reviews.


Learnings added
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1428
File: packages/payment-processor/src/payment/near-conversion.ts:20-21
Timestamp: 2024-07-15T14:13:22.837Z
Learning: Ensure that `mockedNearWalletConnection` in test files uses the `NearProvider` type instead of `any`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand how a more precise type for a mocked value in the tests positively impacts the library. Can you explain? I'll merge this one because of emergencies and the feedback is not related to the core of my change, but happy to later improve.

Copy link

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

*/
export async function payNearConversionRequest(
request: ClientTypes.IRequestData,
Expand All @@ -37,13 +38,7 @@ export async function payNearConversionRequest(
throw new UnsupportedCurrencyError(request.currencyInfo);
}

if (!paymentReference) {
throw new Error('Cannot pay without a paymentReference');
}

if (!network || !NearChains.isChainSupported(network)) {
throw new Error('Should be a Near network');
}
yomarion marked this conversation as resolved.
Show resolved Hide resolved
validatePaymentReference(paymentReference);
NearChains.assertChainSupported(network);

const amountToPay = getAmountToPay(request, amount).toString();
Expand Down
9 changes: 3 additions & 6 deletions packages/payment-processor/src/payment/near-fungible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@ import {
processNearFungiblePayment,
} from './utils-near';
import { NearChains } from '@requestnetwork/currency';
import { validatePaymentReference } from '../utils/validation';

/**
* Processes the transaction to pay a request in fungible token on NEAR with fee (Erc20FeeProxy).
* @param request the request to pay
* @param walletConnection the Near provider.
*/
export async function payNearFungibleRequest(
request: ClientTypes.IRequestData,
Expand All @@ -27,13 +29,8 @@ export async function payNearFungibleRequest(
const { paymentReference, paymentAddress, feeAddress, feeAmount, network } =
getRequestPaymentValues(request);

if (!paymentReference) {
throw new Error('Cannot pay without a paymentReference');
}
validatePaymentReference(paymentReference);

if (!network || !NearChains.isChainSupported(network)) {
throw new Error('Should be a Near network');
}
NearChains.assertChainSupported(network);

const amountToPay = getAmountToPay(request, amount).toString();
Expand Down
7 changes: 3 additions & 4 deletions packages/payment-processor/src/payment/near-input-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ import {
} from './utils';
import { INearTransactionCallback, processNearPayment } from './utils-near';
import { NearChains } from '@requestnetwork/currency';
import { validatePaymentReference } from '../utils/validation';

/**
* processes the transaction to pay a Near request.
* @param request the request to pay
* @param walletConnection the Web3 provider, or signer. Defaults to window.ethereum.
* @param walletConnection the Near provider.
* @param amount optionally, the amount to pay. Defaults to remaining amount of the request.
*/
export async function payNearInputDataRequest(
Expand All @@ -34,9 +35,7 @@ export async function payNearInputDataRequest(
const { paymentReference, paymentAddress } = getRequestPaymentValues(request);
const amountToPay = getAmountToPay(request, amount).toString();
const version = getPaymentExtensionVersion(request);
if (!paymentReference) {
throw new Error('Cannot pay without a paymentReference');
}
validatePaymentReference(paymentReference);

return processNearPayment(
walletConnection,
Expand Down
16 changes: 7 additions & 9 deletions packages/payment-processor/src/payment/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,21 +353,19 @@ export function validateERC20TransferableReceivable(
}

/**
* Computes the amount to pay.
* If `amount` is specified, it will return it.
* Otherwise, it will return the amount left to pay in the request.
* It returns the amount left to pay in the request, unless an amount is specified.
*
* @param request the request to pay
* @param amount the optional amount to pay.
* @param request the request to pay.
* @param amount optionally override the returned amount to pay, in request currency.
* @returns the amount to pay, in request currency.
*/
export function getAmountToPay(
request: ClientTypes.IRequestData,
amount?: BigNumberish,
): BigNumber {
const amountToPay =
amount === undefined
? BigNumber.from(request.expectedAmount).sub(request.balance?.balance || 0)
: BigNumber.from(amount);
const amountToPay = amount
? BigNumber.from(amount)
: BigNumber.from(request.expectedAmount).sub(request.balance?.balance || 0);

if (amountToPay.lt(0)) {
throw new Error('cannot pay a negative amount');
Expand Down
8 changes: 8 additions & 0 deletions packages/payment-processor/src/utils/validation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/** Validates the presence of the payment reference for payment. */
export function validatePaymentReference(
paymentReference?: string,
): asserts paymentReference is string {
if (!paymentReference) {
throw new Error('Cannot pay without a paymentReference');
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ describe('payNearWithConversionRequest', () => {

await expect(
payNearConversionRequest(invalidRequest, mockedNearWalletConnection, conversionSettings),
).rejects.toThrowError('Should be a Near network');
).rejects.toThrowError('Unsupported chain unknown-network');
expect(paymentSpy).not.toHaveBeenCalled();
});
});