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.

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

Frontend: fix bugs in Bitkeep mobile wallet #1559

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Conversation

manuelwedler
Copy link
Contributor

Closes #1513

Ensures that no transfers are shown mistakenly as failing with Bitkeep. During testing I also found that the chain switching does not work properly with Bitkeep. This is also fixed. See commit messages for details.

…rsion to a request

This is a monkeypatch to the function. It is necessary because Bitkeep uses a buggy version of the window.ethereum API. Only some rpcs like the ones of optimism require the version field.
Checks the chainId of the provider and copies its value after a chain switch triggered by the dapp. Also moves the network switch listening to our EthereumProvider, because I think this logic has no relation to the RequestDialog where it was before. Also now uses the network event of ethers.js. This was more reliable than the one of the window.ethereum API.
@manuelwedler manuelwedler changed the title Frontend: fix bugs in Frontend: fix bugs in Bitkeep mobile wallet Mar 10, 2023
@GabrielBuragev GabrielBuragev self-requested a review March 10, 2023 17:13
Copy link
Contributor

@GabrielBuragev GabrielBuragev left a comment

Choose a reason for hiding this comment

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

Everything looks good. Thanks for all the effort around this issue.
I just have one question and one concern outside of the scope of this issue.

I tried testing the app with multiple wallet providers (TrustWallet, BitKeep, TokenPocket) by opening app.beamerbridge.com inside their WebView and it seems like all of them are showing different behaviors. Most noticeable:

  1. TrustWallet is not showing the right wallet connection options (missing MetaMask option)
  2. All of them are not emitting any events when network change happens which makes the app unusable / prone to errors when trying to submit a transfer

We can create a follow up issue for investigating this further as i believe this needs some attention.

this.chainId.value = newNetwork.chainId;
if (oldNetwork) {
window.location.reload();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am trying to figure out why the location.reload() was moved in the ethereum-provider.ts.

Was the intention to substitute the removed watcher from RequestDialog.vue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, it was before in the RequestDialog and I think this logic has no relation to the component. It is only dependent on the provider and that's why I think it should also be placed there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, seems reasonable

@manuelwedler
Copy link
Contributor Author

Are you sure they are not emitting any events? I could verify that BitKeep is throwing the event, but the call to location.reload does nothing. See the comment in the code. If this happens with multiple wallets, maybe it is related to the in-app browser that is probably somewhat based on the Android chrome browser for all apps. Not sure how these in-app browsers work exactly.

@GabrielBuragev
Copy link
Contributor

You are right, the event emitting is not the problem here but rather the location.reload not working properly. Thanks for the effort and for creating the follow up issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User Transfer's Failing on Beamer dApp
2 participants