Skip to content
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.

Sign up for GitHub

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

fix(WalletConnect): Fixing sign for uniswap and paraswap #16168

Merged
merged 1 commit into from
Aug 26, 2024
Merged

fix(WalletConnect): Fixing sign for uniswap and paraswap #16168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion storybook/qmlTests/tests/tst_DAppsWorkflow.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ Item {
verify(eip155.hasOwnProperty("methods"))
verify(eip155.methods.length > 0)
verify(eip155.hasOwnProperty("events"))
compare(eip155.events.length, 2)
compare(eip155.events.length, 5)
}

function test_getAccountsInSession() {
Expand Down
2 changes: 1 addition & 1 deletion ui/app/AppLayouts/Wallet/services/dapps/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ function buildSupportedNamespaces(chainIds, addresses, methods) {
"eip155":{
"chains": [${eipChainIds.join(',')}],
"methods": [${methodsStr}],
"events": ["accountsChanged", "chainChanged"],
"events": ["chainChanged","accountsChanged","message","disconnect","connect"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saledjenic @clauxx Do you know if we'll need additional implementations in the integration layer for these extra events? Can't find anything meaningful in docs, integration or testing.

Copy link
Member

Choose a reason for hiding this comment

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

do you mean the "chainChanged" and "accountsChanged" events? we don't use them as of know from what I know, and doesn't seem like we need them, since we agree ahead of time with the dapp on the account/chains. e.g. whenever the dApp changes the chain, we don't need to react to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm referring to "message","disconnect","connect". The Paraswap pairing fails because they mark these evens as required and we don't declare them as supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

whenever the dApp changes the chain, we don't need to react to that

Really? Didn't know that was our intention.

Do you know if we'll need additional implementations in the integration layer for these extra events?

When the dapp asks for supported namespaces it actually asks if our wallet supports all events/methods that the dapp needs for functionating. In order to connect to the dapp we should at least support "required" events/methods, otherwise that dapp won't be successfully connected to our wallet.

Now if we extend namespace we want to support, we have to add necessary listeners for those events (or connect them to appropriate handlers if already added) and also extend functionality with newly added methods to support all that the namespaces said.

At least that's how I see it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saledjenic Looks like we're being called through methods and we're supposed to call WC APIs through events. https://docs.walletconnect.com/walletkit/web/usage#emitting-session-events

IDK why Paraswap requires us to send all these events, but I think it's up to us to decide when we should send these events. The difficulty I see is to find the appropriate when to send these events.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's how it should be, we have to listen for events sent from the dapp and react to them and notify the dapp about the change on the wallet side.

During the negotiation process, when we're building a namespace, we should always respond with all chains/events/actions we're supporting in our wallet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense! I'll add the events to enable the connection and add another task to go deeper in each signal. Looks like we're missing more handlers, not just these here and it's probably worth it's own space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#16180

"accounts": [${eipAddresses.join(',')}]
}
}`
Expand Down
2 changes: 1 addition & 1 deletion vendor/status-go
Open in desktop
Submodule status-go updated 84 files
+1 −1 Makefile
+0 −1 VERSION
+1 −1 _assets/ci/Jenkinsfile
+8 −2 _assets/ci/Jenkinsfile.android
+1 −1 _assets/ci/Jenkinsfile.docker
+9 −4 _assets/ci/Jenkinsfile.ios
+8 −2 _assets/ci/Jenkinsfile.linux
+41 −13 _assets/ci/Jenkinsfile.tests
+8 −5 _assets/ci/Jenkinsfile.tests-rpc
+0 −62 _assets/ci/validate-vendor.sh
+1 −0 _assets/scripts/tag_version.sh
+1 −1 _assets/scripts/update-fleet-config.sh
+2 −0 _assets/scripts/version.sh
+7 −0 api/geth_backend.go
+4 −0 api/old_mobile_user_upgrading_from_v1_to_v2_test.go
+54 −28 circuitbreaker/circuit_breaker.go
+53 −0 circuitbreaker/circuit_breaker_test.go
+0 −1 cmd/statusd/main.go
+0 −25 config/cli/fleet-status.test.json
+13 −11 eth-node/bridge/geth/waku.go
+20 −13 eth-node/bridge/geth/wakuv2.go
+1 −1 eth-node/types/topic.go
+28 −15 eth-node/types/waku.go
+2 −2 go.mod
+4 −4 go.sum
+13 −3 multiaccounts/accounts/database.go
+0 −1 params/cluster.go
+2 −2 params/version.go
+0 −19 params/version_test.go
+1 −1 protocol/communities_messenger_token_permissions_test.go
+3 −2 protocol/messenger_config_test.go
+6 −0 protocol/messenger_mailserver.go
+7 −1 protocol/messenger_mailserver_cycle.go
+11 −8 protocol/messenger_peers.go
+22 −15 protocol/messenger_storenode_comunity_test.go
+17 −15 protocol/messenger_storenode_request_test.go
+4 −2 protocol/messenger_testing_utils.go
+11 −2 protocol/storenodes/database.go
+8 −3 protocol/storenodes/database_test.go
+18 −12 protocol/storenodes/models.go
+30 −4 protocol/storenodes/storenodes_test.go
+14 −12 protocol/transport/transport.go
+9 −2 rpc/chain/client.go
+22 −12 rpc/client.go
+34 −12 services/ext/api.go
+10 −6 services/mailservers/database.go
+0 −18 services/mailservers/fleet.go
+1 −1 services/wallet/api.go
+19 −6 services/wallet/async/scheduler.go
+36 −1 services/wallet/async/scheduler_test.go
+7 −1 services/wallet/collectibles/manager_test.go
+7 −72 services/wallet/reader.go
+1 −1 services/wallet/reader_test.go
+2 −2 services/wallet/router/errors.go
+4 −0 services/wallet/router/pathprocessor/processor_erc721.go
+8 −8 services/wallet/router/router_v2.go
+1 −0 services/wallet/service.go
+7 −7 services/wallet/thirdparty/coingecko/client.go
+5 −5 services/wallet/thirdparty/coingecko/client_test.go
+9 −5 services/wallet/thirdparty/cryptocompare/client.go
+17 −0 services/wallet/thirdparty/cryptocompare/client_test.go
+7 −0 services/wallet/transfer/downloader.go
+47 −0 services/wallet/transfer/downloader_test.go
+0 −24 static/bindata.go
+2 −1 telemetry/client_test.go
+20 −102 vendor/github.com/ethereum/go-ethereum/signer/core/apitypes/types.go
+4 −0 vendor/github.com/waku-org/go-waku/logging/logging.go
+44 −26 vendor/github.com/waku-org/go-waku/waku/v2/api/missing/missing_messages.go
+18 −11 vendor/github.com/waku-org/go-waku/waku/v2/api/publish/message_check.go
+170 −0 vendor/github.com/waku-org/go-waku/waku/v2/api/publish/message_sender.go
+11 −4 vendor/github.com/waku-org/go-waku/waku/v2/api/publish/rate_limiting.go
+42 −30 vendor/github.com/waku-org/go-waku/waku/v2/node/keepalive.go
+5 −5 vendor/github.com/waku-org/go-waku/waku/v2/node/wakunode2.go
+3 −3 vendor/github.com/waku-org/go-waku/waku/v2/peermanager/peer_connector.go
+33 −1 vendor/github.com/waku-org/go-waku/waku/v2/protocol/peer_exchange/protocol.go
+4 −2 vendor/github.com/waku-org/go-waku/waku/v2/utils/logger.go
+2 −2 vendor/modules.txt
+7 −4 wakuv2/common/filter.go
+315 −0 wakuv2/common/filter_test.go
+2 −1 wakuv2/common/message.go
+29 −74 wakuv2/message_publishing.go
+58 −0 wakuv2/nwaku.go
+78 −99 wakuv2/waku.go
+60 −14 wakuv2/waku_test.go