chore: refactor payment network IDs for enforced typing #977

yomarion · 2022-11-02T14:50:36Z

Description of the changes

In order to type strongly the Payment Network creation parameters, I had to change the way we declare Payment Network IDs.
This highlighted a few type mistakes, potentially runtime mistakes, for example for the type of from (cf. comments).

This strong typing would have avoided the mistake fixed by this previous change.

chore: enforce the typing of creation parameters with the type PaymentNetworkCreateParameters
chore: deprecate the IPaymentNetworkCreateParameters interface
chore: remove enum PaymentTypes.PAYMENT_NETWORK_ID
chore: created enums ExtensionTypes.OTHER_ID and ExtensionTypes.PAYMENT_NETWORK_ID, ExtensionTypes.ID is defined by their union

Note for validators

Most changes are automatic replacements as explained below.
The main changes are highlighted by comments.

(Type) Breaking change and notice

This change is type-breaking but not runtime-breaking, as the enum values are the same.
For uses of PaymentTypes.PAYMENT_NETWORK_ID.<SOME_ID>, they can be easily replaced with ExtensionTypes.PAYMENT_NETWORK_ID.<SOME_ID>, except for DECLARATIVE and ANY_TO_NATIVE that are named ANY_DECLARATIVE and ANY_TO_NATIVE_TOKEN.
(example 1 and example 2)

References to ExtensionTypes.ID.PAYMENT_NETWORK_<SOME_ID> should be replaced with ExtensionTypes.PAYMENT_NETWORK_ID.<SOME_ID>.
(example)

References to IPaymentNetworkCreateParameters interface should be replaced with PaymentNetworkCreateParameters type.

yomarion · 2022-11-03T07:03:50Z

packages/payment-detection/src/any-to-any-detector.ts

 export abstract class AnyToAnyDetector<
  TExtension extends ExtensionTypes.PnFeeReferenceBased.IFeeReferenceBased,
-  TPaymentEventParameters,
+  TPaymentEventParameters extends Partial<ExtensionTypes.PnFeeReferenceBased.IAddFeeParameters>,


Here is an example of a hidden mistyping that cannot be missed anymore.

yomarion · 2022-11-03T07:04:08Z

packages/payment-detection/src/any-to-native-detector.ts

 export abstract class AnyToNativeDetector extends AnyToAnyDetector<
  ExtensionTypes.PnAnyToEth.IAnyToEth,
-  PaymentTypes.IETHPaymentEventParameters
+  PaymentTypes.IETHFeePaymentEventParameters


Here is an example of a hidden mistyping that cannot be missed anymore.

yomarion · 2022-11-03T07:06:28Z

packages/payment-detection/src/declarative.ts

 export abstract class DeclarativePaymentDetectorBase<
  TExtension extends ExtensionTypes.PnAnyDeclarative.IAnyDeclarative,
-  TPaymentEventParameters extends PaymentTypes.IDeclarativePaymentEventParameters,
+  TPaymentEventParameters extends PaymentTypes.IDeclarativePaymentEventParameters<unknown>,


Here is an example of a hidden mistyping that cannot be missed anymore.
More specifically, Declarative payment events are at the root of many detectors, but when events are declarative for real, they use IIdentity type for from, whereas it's string for overriding children (when it's not declarative)

yomarion · 2022-11-03T07:06:47Z

packages/payment-detection/src/fee-reference-based-detector.ts

 export abstract class FeeReferenceBasedDetector<
  TExtension extends ExtensionTypes.PnFeeReferenceBased.IFeeReferenceBased,
-  TPaymentEventParameters extends { feeAmount?: string; feeAddress?: string },
+  TPaymentEventParameters extends PaymentTypes.IDeclarativePaymentEventParameters<string> & {


Here is an example of a hidden mistyping that cannot be missed anymore.

yomarion · 2022-11-03T07:08:35Z

packages/types/src/client-types.ts

  requestInfo: RequestLogic.ICreateParameters | IRequestInfo;
  signer: Identity.IIdentity;
-  paymentNetwork?: Payment.IPaymentNetworkCreateParameters;
+  paymentNetwork?: Payment.PaymentNetworkCreateParameters;


Main change: from interface to type

yomarion · 2022-11-03T07:09:40Z

packages/types/src/extension-types.ts

-  PAYMENT_NETWORK_ANY_TO_ETH_PROXY = 'pn-any-to-eth-proxy',
 }

+export enum PAYMENT_NETWORK_ID {


Strong impact required for the main change: instead of mapping a new enum out of ID to qualify payment network IDs, we now have 2 enums.

yomarion · 2022-11-03T07:11:20Z

packages/types/src/payment-types.ts

+  parameters: T;
+}
+
+export type PaymentNetworkCreateParameters =


This is the main change. We strongly type creation parameters based on the different payment networks.

yomarion · 2022-11-03T07:21:50Z

packages/types/src/payment-types.ts

+    };
+
+/** Payment network IDs with known Create Parameters */
+export type PAYMENT_NETWORK_ID = PaymentNetworkCreateParameters['id'];


This is not an enum anymore, and is probably useless outside PaymentTypes.

yomarion · 2022-11-03T07:22:56Z

packages/types/src/payment-types.ts


 /** Parameters for events of Declarative payments */
-export interface IDeclarativePaymentEventParameters {
+export interface IDeclarativePaymentEventParameters<TFrom = IIdentity> {


(cf. other comment about this interface) Declarative payment events are at the root of many detectors, but when events are declarative for real, they use IIdentity type for from, whereas it's string for overriding children (when it's not declarative)

yomarion · 2022-11-03T07:23:26Z

packages/types/src/payment-types.ts

 }
 /** Parameters for events of ERC777 payments */
-export interface IERC777PaymentEventParameters {
+export interface IERC777PaymentEventParameters extends GenericEventParameters {


Side-improvement. (and a few other similare replacements)

yomarion · 2022-11-03T07:24:34Z

packages/types/src/utils.ts

-  for (const pn in PaymentTypes.PAYMENT_NETWORK_ID) {
-    if (PaymentTypes.PAYMENT_NETWORK_ID[pn] === value) {
+export function isPaymentNetworkId(value: any): value is ExtensionTypes.PAYMENT_NETWORK_ID {
+  for (const pn of enumKeys(ExtensionTypes.PAYMENT_NETWORK_ID)) {


The old trick was not working anymore

I wonder if isPaymentNetworkId is still necessary. It seems to be there to account for the possibility of the ID not being a payment network ID, but now we have a strong type to enforce it?

btw, you could also do Object.values(ExtensionTypes.PAYMENT_NETWORK_ID).includes(value)

I agree, it should probably be removed or at least accept only ID values.

…ds-ble

benjlevesque

great change 👏

packages/payment-processor/src/payment/batch-proxy.ts

packages/payment-processor/src/payment/erc20.ts

benjlevesque · 2022-11-03T10:02:34Z

packages/types/src/utils.ts

-  for (const pn in PaymentTypes.PAYMENT_NETWORK_ID) {
-    if (PaymentTypes.PAYMENT_NETWORK_ID[pn] === value) {
+export function isPaymentNetworkId(value: any): value is ExtensionTypes.PAYMENT_NETWORK_ID {
+  for (const pn of enumKeys(ExtensionTypes.PAYMENT_NETWORK_ID)) {


I wonder if isPaymentNetworkId is still necessary. It seems to be there to account for the possibility of the ID not being a payment network ID, but now we have a strong type to enforce it?

btw, you could also do Object.values(ExtensionTypes.PAYMENT_NETWORK_ID).includes(value)

packages/types/src/utils.ts

benjlevesque · 2022-11-03T10:04:26Z

packages/types/src/payment-types.ts

 }

+/** Interface to create a payment network  */
+export interface IPaymentNetworkCreateParameters<T = any> {


do we still need the default any?

I have actually deprecated this interface in the meantime @benjlevesque

packages/types/src/payment-types.ts

alexandre-abrioux

Nice! I remember wanting to do something like this in my previous refactor but refrained as it was already too many changes 🤩 Awesome!

packages/types/src/payment-types.ts

olivier7delf

great job!

packages/payment-detection/src/payment-network-factory.ts

coveralls · 2022-11-03T19:00:15Z

Coverage decreased (-0.02%) to 88.896% when pulling d0107e5 on chore/refactor-pn-ids-ble into abc53c7 on master.

yomarion added 3 commits November 1, 2022 17:36

wip

396ab91

big replace

c59d483

fix: isPaymentNetworkId

c8c0574

yomarion commented Nov 3, 2022

View reviewed changes

comment

7ab0cae

yomarion commented Nov 3, 2022

View reviewed changes

yomarion marked this pull request as ready for review November 3, 2022 07:33

yomarion requested review from AdamREQ, alexandre-abrioux, benjlevesque, leoslr, olivier7delf and omarspx November 3, 2022 07:36

yomarion added 2 commits November 3, 2022 09:44

Merge remote-tracking branch 'origin/master' into chore/refactor-pn-i…

f3f4370

…ds-ble

deprecate the IPaymentNetworkCreateParameters interface

9fd276e

benjlevesque approved these changes Nov 3, 2022

View reviewed changes

alexandre-abrioux approved these changes Nov 3, 2022

View reviewed changes

packages/types/src/payment-types.ts Show resolved Hide resolved

packages/types/src/payment-types.ts Outdated Show resolved Hide resolved

olivier7delf approved these changes Nov 3, 2022

View reviewed changes

packages/payment-detection/src/payment-network-factory.ts Show resolved Hide resolved

yomarion added 5 commits November 3, 2022 13:07

fix: mistyping in test

f61f35e

fix: http-request-network.test.ts

25f0b7d

PR comments

ea7798d

fix: declarative-payments.test

4ed9c74

fix: some request-client tests

02f0b28

Merge branch 'master' into chore/refactor-pn-ids-ble

d0107e5

yomarion merged commit 796d3f4 into master Nov 3, 2022

yomarion deleted the chore/refactor-pn-ids-ble branch November 3, 2022 21:38

alexandre-abrioux mentioned this pull request May 9, 2023

fix(integration-tests): unskip tests #1107

Merged

chore: refactor payment network IDs for enforced typing #977

chore: refactor payment network IDs for enforced typing #977

Uh oh!

Conversation

yomarion commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the changes

Note for validators

(Type) Breaking change and notice

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjlevesque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexandre-abrioux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

olivier7delf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yomarion commented Nov 2, 2022 •

edited

Loading

coveralls commented Nov 3, 2022 •

edited

Loading