-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat(wallet-connect): switch to the safe-apps-provider under the hood #537
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
1e3b960
to
6370dbf
Compare
ESLint Summary View Full Report
Report generated by eslint-plus-action |
result = safeTxHash | ||
break | ||
} | ||
case 'gs_multi_send': { |
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 supose this was a test method that wasn't still implemented?
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.
This is the method for batching transactions implemented by the OG app. I'm not sure if it's widely used. Quick GitHub search shows 1-2 apps: https://github.com/search?q=gs_multi_send&type=code
I think the mobile app continues to support it, so if we want to keep the support, we can implement it in the provider
} | ||
|
||
let result = await web3Provider.send(payload.method, payload.params) | ||
console.log({ result }) |
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.
Remember to remove this 😄
@dasanra there are some challenges related to the |
6370dbf
to
352f6b9
Compare
ESLint Summary View Full Report
Report generated by eslint-plus-action |
This came out as an attempt to fix #377
What it solves
It switches the walletconnect Safe App to the
safe-apps-provider
package, so in case we change the handling of any of the RPC requests for the Safe Apps we can simply update it once in the SDK and bump the package versionHow to test it
Connect to your favourite app, transact with it and make sure everything is sound
Why #377 wasn't solved
The problem is that in the case of walletconnect most of the wallet libraries are using the wallet provider only for WRITE requests, and READ requests are performed with a separate provider:
Therefore we cannot easily make it work with walletconnect. Further, I see three options:
I’m personally leaning towards 3) as the easiest option