Conversation
| tokenVaultV3Contract, | ||
| fallbackProvider, | ||
| pollingProvider, | ||
| undefined, // pollingProvider is being deprecated |
There was a problem hiding this comment.
Type error here. I do not think it's a breaking change to change the type to ? on the functions that take the now deprecated polling provider such that you no longer have to pass them
There was a problem hiding this comment.
Are you saying to remove the optional param and not pass undefined? Or just stating that it is okay
It is part of the last commit to the relating engine PR Railgun-Community/engine@3e3f607
| /** | ||
| * @deprecated pollingInterval - DEPRECATED | ||
| */ | ||
| pollingInterval: number, |
There was a problem hiding this comment.
It's already there if you are asking to ensure it has deprecated on it
bhflm
left a comment
bhflm
left a comment
There was a problem hiding this comment.
minor suggestions feel free to apply
…r engine.loadNetwork call, ensure test catches invalid provider json
|
TODO: This contains a shared-models patch from Railgun-Community/shared-models#41 - update version when it is merged Allows pollingInterval to be passed with FallbackProvider |
|
TODO: This contains an engine patch from Railgun-Community/engine#112 - update version when it is merged Allows for type WebSocketProvider to be passed into engine |
…on does not need to specify it
|
TODO: |
… provider creation function
| "lint": "npm run check-circular-deps && npm run eslint && npm run tsc && npm run tsc-test", | ||
| "prepare": "npm run build", | ||
| "postinstall": "node postinstall.js", | ||
| "postinstall": "node postinstall.js && patch-package", |
There was a problem hiding this comment.
we plan to remove this once the pr is approved right ?
| providerJsonConfig: FallbackProviderJsonConfig | ProviderJson, | ||
| pollingInterval?: number, | ||
| ): Promise<Provider> => { | ||
| const existingProvider = providerMap[networkName]; |
There was a problem hiding this comment.
It's throwing a lint error for me here
Unexpected any value in conditional. An explicit comparison or type cast is required
| @@ -223,11 +217,20 @@ export const resumeIsolatedPollingProviderForNetwork = ( | |||
| pauseAllPollingProviders( | |||
| networkName, // excludeNetworkName | |||
There was a problem hiding this comment.
can we rename this to excludedNetworkName instead of just having it as a comment ?
| } | ||
|
|
||
| // Ensure provider has resume functionality | ||
| if (!('paused' in provider && 'resume' in provider)) { |
There was a problem hiding this comment.
We could do a validateProvider function rather than having this statement twice, since the flow itself its pretty similar
something like
type PausableProvider extends Provider {
pause(): voidl;
resume(): void;
paused: boolean;
}
function isPausableProvider(provider: Provider): provider is PausableProvider {
return 'pause' in provider && 'resume' in provider && 'paused' in provider;
}
const validateProvider = (networkName: NetworkName, provider?: Provider) => {
if (!isDefined(provider)) {
throw new Error(`No provider found for network: ${networkName}`);
}
if (!isPausableProvider(provider)) {
throw new Error(`Provider for network ${networkName} is not a pausable provider`);
}
return provider;
};
bhflm
left a comment
bhflm
left a comment
There was a problem hiding this comment.
LGTM, minor comments on nice to have refactors
3 participants
Problem
Polling provider is deprecated in engine PR Railgun-Community/engine#112
Solution
This helps remove call to engine.loadNetwork() with a pollingProvider, as the engine PR does not use polling provider. It only allows passing it now to prevent a breaking change and allow time for full deprecation.