Skip to content

Conversation

@yomarion
Copy link
Member

@yomarion yomarion commented Mar 19, 2023

Description of the changes

ERC20FeeProxy extension supports Near fungible token transfers, in advanced-logic, and payment-detection.

The ERC20FeeProxy artifacts now refer to a version named near, aside 0.1.0 etc. This choice can be debated but the assumption is that we can update contracts on Near without changing their address.

  • chore: getFeeProxyContractErc20ForNetwork method in the AdvancedLogic object

Other changes

In @requestnetwork/currency:

In @requestnetwork/types and @requestnetwork/smart-contracts:

  • chore: CurrencyTypes.VMChainName: Virtual machin chains, where payment proxy contracts can be deployed
  • chore: ArtifactInfo type now supports NearChains, except for the connect method that only supports EvmChainName
  • chore: artifacts can refer to Near deployment addresses

In @requestnetwork/payment-detection:

  • chore: some tests use the actual advanced-logic instead of mocks, to reduce mock maintenance. I don't think it impacts performances, maybe unit testing isolation but advanced-logic and payment-detection are strongly coupled anyway.

Credits

🙌 Shoutouts to @alexandre-abrioux for paving the way with #1056 and #950.


/** Extension interface is extended by the extensions implementation */
export interface IExtension<T = any> {
export interface IExtension<TCreationParameters = any> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor unrelated clarification

Copy link
Member Author

Choose a reason for hiding this comment

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

TODO this yarn lock update is probably irrelevant or wrong

@coveralls
Copy link

coveralls commented Mar 22, 2023

Coverage Status

Coverage: 87.584% (+0.02%) from 87.566% when pulling 87a1207 on feat/erc20-fee-proxy-on-near into 0629e45 on master.

@yomarion yomarion mentioned this pull request Mar 23, 2023
@yomarion yomarion marked this pull request as ready for review March 27, 2023 10:55
@yomarion yomarion requested review from benjlevesque and leoslr and removed request for alexandre-abrioux, kevindavee and olivier7delf March 27, 2023 10:56
Copy link
Contributor

@leoslr leoslr left a comment

Choose a reason for hiding this comment

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

I could not find any blocker.
Just a general, question, a saw that most of the test were using aurora / aurora-testnet. Are we sure they would pass as well using near / near-testnet ?
Or that is planned for later ?

@@ -1,5 +1,5 @@
overwrite: true
schema: 'https://api.thegraph.com/subgraphs/name/requestnetwork/request-payments-near'
schema: 'https://api.thegraph.com/subgraphs/name/requestnetwork/request-payments-near-testnet'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using the testnet ?

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 have not deployed to mainnet yet, but I can actually. I noticed that for EVMs the schema is pulled from goerli.

Copy link
Contributor

@alexandre-abrioux alexandre-abrioux Mar 28, 2023

Choose a reason for hiding this comment

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

@leoslr I'm guessing it is to retrieve the latest schema, even before it is deployed to mainnet

@yomarion
Copy link
Member Author

I could not find any blocker. Just a general, question, a saw that most of the test were using aurora / aurora-testnet. Are we sure they would pass as well using near / near-testnet ? Or that is planned for later ?

Yes, we plan to make it a different change. With the recent strong typing of NearChains, it should be quite easy, we can use type the actual NearChains and NearChainsAlias differently, and little by little reduce the support surface for NearChainsAlias.

Thanks for your review @leoslr

Copy link
Contributor

@alexandre-abrioux alexandre-abrioux left a comment

Choose a reason for hiding this comment

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

Awesome changes, I like the new chain helpers! 🙂
And thank you for the shoutout 😇

@@ -1,5 +1,5 @@
overwrite: true
schema: 'https://api.thegraph.com/subgraphs/name/requestnetwork/request-payments-near'
schema: 'https://api.thegraph.com/subgraphs/name/requestnetwork/request-payments-near-testnet'
Copy link
Contributor

@alexandre-abrioux alexandre-abrioux Mar 28, 2023

Choose a reason for hiding this comment

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

@leoslr I'm guessing it is to retrieve the latest schema, even before it is deployed to mainnet

anyToNativeTokenExtension.supportedNetworks.includes(network),
);
network?: CurrencyTypes.ChainName,
): AnyToNative | undefined {
Copy link
Member Author

Choose a reason for hiding this comment

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

(Not related) Return type simplification for maintainability

@@ -1,5 +1,5 @@
overwrite: true
schema: 'https://api.thegraph.com/subgraphs/name/requestnetwork/request-payments-near'
schema: 'https://api.thegraph.com/subgraphs/name/requestnetwork/request-payments-near-testnet'
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 have not deployed to mainnet yet, but I can actually. I noticed that for EVMs the schema is pulled from goerli.

@yomarion yomarion enabled auto-merge (squash) March 30, 2023 15:46
@yomarion yomarion merged commit 6ac8c28 into master Mar 30, 2023
@yomarion yomarion deleted the feat/erc20-fee-proxy-on-near branch March 30, 2023 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants