-
Notifications
You must be signed in to change notification settings - Fork 91
refactor(advanced-logic): add evm-based PN #945
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
Conversation
| public extensionType: ExtensionTypes.TYPE, | ||
| public extensionId: ExtensionTypes.ID, | ||
| public currentVersion: string, | ||
| protected constructor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constructors on abstract classes be protected
benjlevesque
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is even required...
having the network hard coded in the library feels wrong;
could we rely on smart-contracts for that? or be more permissive?
|
@alexandre-abrioux the advanced logic doesn't depend on smart-contracts. Maybe the solution would be to move the check on the network/version in the request-client? Or, maybe easier, in payment-detection. |
| public readonly extensionId: ExtensionTypes.ID = ExtensionTypes.ID | ||
| .PAYMENT_NETWORK_ERC777_STREAM, | ||
| public readonly currentVersion: string = CURRENT_VERSION, | ||
| public readonly supportedNetworks: string[] = EvmBasedPaymentNetwork.EVM_NETWORKS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexandre-abrioux ERC777 is not supported on all networks, most importantly mainnet is not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KolevDarko I took a different approach in the end as advised by Benji. I'm working on it here: #950
This PR adds a composable
evm-basedpayment network. It decouples the advanced-logic layer from the current state of deployed smart contracts. See discussion: #943 (comment)This PR, if merged, would supersede the following: #943