refactor: check for contract deployment before creating pn #950

alexandre-abrioux · 2022-10-11T20:23:26Z

supersedes #945 and #943

Context

The advanced-logic layer is a stateless singleton that contains an instance of all payment networks (PN). When created (see its class), it has no knowledge of what chain(s) it will be used on. It only needs an instance of the currency manager. It has no other dependency.

Issue / Solution

It is a pain to manually update PN whenever we want them to support a new chain, especially when it could be done automatically via our smart contract artifacts. Without adding new dependencies on the advanced-logic object, this can be done from the payment-detection layer. PN should have zero knowledge of what chain(s) they support. Routing from the chain/version to the payment network should be done during the instantiation of the payment detector.

From this point:

isValidAddress in PNs now only accepts an address (no more network parameter), this means:
- PN supporting EVM chains then support EVM addresses (that was already the case)
- PN can now only support one type of address, so all Near-related PN had to be split in two: one for Near mainnet and one for Near testnet, the same way we already had one for BTC and one for BTC-testnet
PN don't accept a supportedNetworks parameter anymore. Classes that extend native-token and any-to-native still have a supportedNetworks array as they need some kind of deeper level of routing. However, those are supper specific and not supposed to change that often once they are set.
everything Near related has been moved to its own directory in advanced-logic, like we did in payment-detection
the advanced-logic package typing has been updated (removed any whenever possible)

Ideally, those changes should not impact the behavior of the library when used for payment detection.

alexandre-abrioux · 2022-10-11T20:25:07Z

packages/advanced-logic/src/extensions/content-data.ts

-  public constructor(
-    public extensionId: ExtensionTypes.ID = ExtensionTypes.ID.CONTENT_DATA,
-    public currentVersion: string = CURRENT_VERSION,
-  ) {
-    super(ExtensionTypes.TYPE.CONTENT_DATA, extensionId, currentVersion);
+  public constructor() {
+    super(ExtensionTypes.TYPE.CONTENT_DATA, ExtensionTypes.ID.CONTENT_DATA, CURRENT_VERSION);


Type of change made for classes that never get extended

alexandre-abrioux · 2022-10-11T20:27:00Z

packages/advanced-logic/src/extensions/payment-network/any-to-native.ts


 export default abstract class AnyToNativeTokenPaymentNetwork extends FeeReferenceBasedPaymentNetwork {
-  public constructor(
+  protected constructor(


abstract class constructors should be protected

coveralls · 2022-10-13T18:22:23Z

Coverage decreased (-0.1%) to 88.999% when pulling e57154b on pn-check-contract-deployed into 7055d4c on master.

…eployed

alexandre-abrioux · 2022-10-14T11:49:43Z

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

+/**
+ * Handle payment detection for native token payment with conversion
+ */
+export abstract class AnyToNativeDetector extends AnyToAnyDetector<


New abstract class for all (future) any-to-native. Did the same for native-token

alexandre-abrioux · 2022-10-14T12:20:00Z

packages/payment-detection/src/erc20/address-based.ts

 import Erc20InfoRetriever from './address-based-info-retriever';

 import { PaymentDetectorBase } from '../payment-detector-base';
-const supportedNetworks = ['mainnet', 'rinkeby', 'goerli', 'private'];


Please challenge this. Here we're in the payment-detection package, so the context is not exactly the same. I don't think it makes sense to have a fixed array of supportedNetworks here either, but I may be wrong.

Yes, I agree. If there was such a limitation, it would be in the info-retriever.

Maybe throwing if no (default) provider exists for the network, within getTransferEvents(), would maintain clarity for what works and what does not? And the next step depends on what this provider returns if we query a wider range of blocks than it allows, specifically, if it allows {fromBlock: 0, toBlock: 'latest'}.

As we expect the result to be incomplete in most cases, while getTransferEvents() should be TheGraph-based, shouldn't we add a console.warn for address-based info retrieval?

Good idea! Let me try to do something about that.

After further digging, I found out that getDefaultProvider already throws whenever a provider cannot be instantiated for the network, so that was already taken care of! This throws:

requestNetwork/packages/payment-detection/src/provider.ts

Line 97 in 8675519

return providers.getDefaultProvider(network, providersApiKeys);

As you advised I added a warning every time we use getTransferEvents from an ERC20InfoRetriever, done here: 8675519 👍

alexandre-abrioux · 2022-10-14T12:22:53Z

packages/payment-detection/src/near/near-conversion-detector.ts

-  }) {
-    super(
-      PaymentTypes.PAYMENT_NETWORK_ID.ANY_TO_NATIVE,
-      advancedLogic.extensions.anyToNativeToken[0],


No need to specify an index like this anymore. The routing is now done automatically in the constructor of the new parent class AnyToNativeDetector, using the new getAnyToNativeTokenExtensionForNetwork method of the AdvancedLogic class

alexandre-abrioux · 2022-10-14T12:25:39Z

packages/payment-detection/src/payment-detector-base.ts

-    protected readonly extension: TExtension,
+  protected constructor(
+    public readonly paymentNetworkId: PaymentTypes.PAYMENT_NETWORK_ID,
+    public readonly extension: TExtension,


public because now needed in the factory

alexandre-abrioux · 2022-10-14T12:26:22Z

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

+    if (detector.extension && 'getDeploymentInformation' in detectorClass) {
+      // this throws when the contract isn't deployed and was mandatory for payment detection
+      (detectorClass as ContractBasedDetector).getDeploymentInformation(
+        network,
+        detector.extension.currentVersion,
+      );
+    }


This is the main change, please review this 😇 We let the detector choose wether we should throw or not for an unsupported network. Do you think that is OK?

alexandre-abrioux · 2022-10-14T15:10:49Z

packages/payment-detection/test/near/near-native-conversion.test.ts

-    expect(paymentNetworkFactory.getPaymentNetworkFromRequest(request)).toBeInstanceOf(
-      NearConversionNativeTokenPaymentDetector,
-    );
+    expect(
+      paymentNetworkFactory.getPaymentNetworkFromRequest({
+        ...request,
+        currency: { ...request.currency, network: 'aurora-testnet' },
+      }),
+    ).toBeInstanceOf(NearConversionNativeTokenPaymentDetector);


The network was missing, and since we're not mocking advancedLogic in this test file anymore, the test wasn't working

alexandre-abrioux · 2022-10-14T15:11:22Z

packages/payment-detection/test/near/near-native.test.ts

    const paymentDetector = new NearNativeTokenPaymentDetector({
-      advancedLogic: mockAdvancedLogic,
+      network: 'aurora',
+      advancedLogic: advancedLogic,
      currencyManager: CurrencyManager.getDefault(),
    });


same as above

alexandre-abrioux · 2022-10-14T15:15:50Z

packages/types/src/advanced-logic-types.ts

+  // FIXME: should be Extension.PnReferenceBased.IReferenceBased<Extension.PnStreamReferenceBased.ICreationParameters>
+  erc777Stream: any;


This was messing with some files if I remember well, an issue between the interface and the implementation. Will try to fix it in a consecutive PR

alexandre-abrioux · 2022-10-14T15:16:14Z

packages/types/src/advanced-logic-types.ts

+  getAnyToNativeTokenExtensionForNetwork: (
+    network: string,
+  ) => Extension.IExtension<Extension.PnAnyToEth.ICreationParameters> | undefined;
+  extensions: IAdvancedLogicExtensions;


Properly typed the extensions property. This way we can quickly know if our PN interfaces (types) match our PN implementations (classes).

alexandre-abrioux · 2022-10-14T15:17:54Z

packages/types/src/extensions/pn-any-declarative-types.ts

    parameters: IAddRefundInstructionParameters,
  ) => Extension.IAction;
-  createCreationAction: (parameters?: TCreationParameters) => Extension.IAction;
+  createCreationAction: (parameters: TCreationParameters) => Extension.IAction;


The interface was not compatible with the actual class implementation. I fixed it that way, let me know if you would have done otherwise.

…eployed

benjlevesque

amazing! great work. Happy to see those supportedNetworks go away 👍

benjlevesque · 2022-10-18T11:09:52Z

packages/payment-detection/src/near/near-conversion-detector.ts

-  }) {
-    super(
-      PaymentTypes.PAYMENT_NETWORK_ID.ANY_TO_NATIVE,
-      advancedLogic.extensions.anyToNativeToken[0],


packages/payment-detection/src/erc20/fee-proxy-contract.ts

packages/payment-detection/src/native-token-detector.ts

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

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

yomarion

Really clean, well done.

yomarion · 2022-10-19T13:23:27Z

packages/advanced-logic/src/extensions/payment-network/near/any-to-near.ts

-
-  private isValidMainnetAddress(address: string): boolean {
+  protected isValidAddress(address: string): boolean {
    return this.isValidAddressForSymbolAndNetwork(address, 'NEAR', 'aurora');


I don't understand why we get rid of the testnet addresses validation. FYI they have different validation rules compared to mainnet.

Near testnet now has its separate class:

requestNetwork/packages/advanced-logic/src/extensions/payment-network/near/any-to-near-testnet.ts

Line 17 in e57b267

return this.isValidAddressForSymbolAndNetwork(address, 'NEAR-testnet', 'aurora-testnet');

yomarion · 2022-10-19T13:37:13Z

packages/payment-detection/src/erc20/address-based.ts

 import Erc20InfoRetriever from './address-based-info-retriever';

 import { PaymentDetectorBase } from '../payment-detector-base';
-const supportedNetworks = ['mainnet', 'rinkeby', 'goerli', 'private'];


Yes, I agree. If there was such a limitation, it would be in the info-retriever.

Maybe throwing if no (default) provider exists for the network, within getTransferEvents(), would maintain clarity for what works and what does not? And the next step depends on what this provider returns if we query a wider range of blocks than it allows, specifically, if it allows {fromBlock: 0, toBlock: 'latest'}.

As we expect the result to be incomplete in most cases, while getTransferEvents() should be TheGraph-based, shouldn't we add a console.warn for address-based info retrieval?

yomarion · 2022-10-19T13:51:27Z

packages/advanced-logic/src/advanced-logic.ts

+      nativeToken: [new NearNative(), new NearTestnetNative()],
+      anyToNativeToken: [new AnyToNear(currencyManager), new AnyToNearTestnet(currencyManager)],


If we go down that line, can we add anyToEthProxy and feeProxyContractEth to these 2 generic PNs? And deprecate the "separate" use of these extensions? It does not add anything while we just have Near.

I agree! We should deprecate any-to-eth-proxy and instead use any-to-native-token. Not sure however that eth-fee-proxy-contract and native-token are compatible as the latest doesn't support fees as far as I know. I'll keep this PR a refactor without changing payment networks, but let's keep this in mind for the next steps.

chore(tsconfig): specify included files during dev

7c57a51

alexandre-abrioux commented Oct 11, 2022

View reviewed changes

alexandre-abrioux added 2 commits October 11, 2022 22:40

WIP

c3f980c

WIP

9e7a533

alexandre-abrioux force-pushed the pn-check-contract-deployed branch from 818873a to 9e7a533 Compare October 11, 2022 20:41

alexandre-abrioux changed the base branch from master to fix-ts-json October 11, 2022 20:42

alexandre-abrioux added 8 commits October 11, 2022 22:44

WIP

0396371

WIP

40aa6ba

fix any-to-near tests

a71afe7

move near to subdir

b627be7

fix tests

37c3813

fix tests

3f1a377

better types

ac8ea1b

WIP

30827dd

Base automatically changed from fix-ts-json to master October 13, 2022 08:44

merge master

74251c9

alexandre-abrioux mentioned this pull request Oct 13, 2022

refactor(advanced-logic): add evm-based PN #945

Closed

alexandre-abrioux added 3 commits October 13, 2022 12:01

fix tests

b472611

fix tests

d7cbe70

fix tests

981d21b

alexandre-abrioux added 8 commits October 14, 2022 10:47

add NETWORK const

5bf88cb

review update

d436294

Merge remote-tracking branch 'origin/master' into pn-check-contract-d…

9d018a1

…eployed

fix after merge

203ef8f

fix after merge

08b65f9

fix aurora test

aef7c7f

fix type

58c1457

fix type

f947d6b

alexandre-abrioux commented Oct 14, 2022

View reviewed changes

alexandre-abrioux marked this pull request as ready for review October 14, 2022 15:21

github-actions bot requested review from kevindavee, leoslr and yomarion October 14, 2022 15:22

alexandre-abrioux requested review from KolevDarko, benjlevesque, bertux and olivier7delf October 14, 2022 15:23

Merge remote-tracking branch 'origin/master' into pn-check-contract-d…

68e59df

…eployed

benjlevesque approved these changes Oct 18, 2022

View reviewed changes

yomarion approved these changes Oct 19, 2022

View reviewed changes

alexandre-abrioux added 4 commits October 19, 2022 16:53

abstract protected constructor

b3916ca

fixes #950 (comment)

e57b267

add warn for ERC20InfoRetriever

8675519

Merge branch 'master' into pn-check-contract-deployed

8b49ce1

alexandre-abrioux added a commit that referenced this pull request Oct 20, 2022

fixes #950 (comment)

ff4aac6

fixes #950 (comment)

e57154b

alexandre-abrioux force-pushed the pn-check-contract-deployed branch from ff4aac6 to e57154b Compare October 20, 2022 16:54

alexandre-abrioux merged commit 25c3ec0 into master Oct 20, 2022

alexandre-abrioux deleted the pn-check-contract-deployed branch October 20, 2022 17:03

alexandre-abrioux mentioned this pull request Oct 20, 2022

fix: IPaymentNetworkState better typing #967

Merged

yomarion mentioned this pull request Oct 31, 2022

fix: request-client should support conversion and network checks #975

Merged

yomarion mentioned this pull request Mar 27, 2023

feat: ERC20FeeProxy on Near testnet #1086

Merged

		// FIXME: should be Extension.PnReferenceBased.IReferenceBased<Extension.PnStreamReferenceBased.ICreationParameters>
		erc777Stream: any;

		nativeToken: [new NearNative(), new NearTestnetNative()],
		anyToNativeToken: [new AnyToNear(currencyManager), new AnyToNearTestnet(currencyManager)],

refactor: check for contract deployment before creating pn #950

refactor: check for contract deployment before creating pn #950

Uh oh!

Conversation

alexandre-abrioux commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Issue / Solution

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

alexandre-abrioux Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yomarion left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alexandre-abrioux commented Oct 11, 2022 •

edited

Loading

coveralls commented Oct 13, 2022 •

edited

Loading

alexandre-abrioux Oct 19, 2022 •

edited

Loading