Feature: Make safe-app-provider follow eip1193 standard #140
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
[warning] @typescript-eslint/explicit-module-boundary-types
[warning] @typescript-eslint/no-explicit-any
Report generated by eslint-plus-action |
|
|
||
| constructor(safe: SafeInfo, sdk: SafeAppsSDK) { | ||
| this.safe = safe; | ||
| this.sdk = sdk; | ||
| } | ||
|
|
||
| public get chainId(): number { | ||
| return NETWORK_CHAIN_ID[this.safe.network]; | ||
| async connect(): Promise<void> { |
There was a problem hiding this comment.
If the implementation of this method returns null, is it following the specification regarding Connectivity?
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1193.md#connectivity
There was a problem hiding this comment.
It doesn't really specify what should be returned. Also, it doesn't return null, it returns nothing (that defaults to undefined). I can see the only possible things are the connect/disconnect events, but I don't think they make much sense for the safe provider because of its somewhat unique usage.
There was a problem hiding this comment.
Also shouldn't we emit the ProviderConnectInfo at least once? (see https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1193.md#connect)
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| sendAsync(request: any, callback: (error: any, response: any) => void): void { | ||
| this.send(request, callback); | ||
| async disconnect(): Promise<void> { |
There was a problem hiding this comment.
If the implementation of this method returns null, is it following the specification regarding Connectivity?
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1193.md#connectivity
There was a problem hiding this comment.
As this provider cannot disconnect, doing nothing is expected here. (Maybe we should comment this)
It turned out that our provider is not https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1193.md compatible. In particular, it was missing events system, and connect/disconnect functionality. This PR fixes it