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

Feature/more wallets #337

Merged
merged 11 commits into from
May 22, 2023
Merged

Feature/more wallets #337

merged 11 commits into from
May 22, 2023

Conversation

ccali11
Copy link
Contributor

@ccali11 ccali11 commented May 19, 2023

@shanejearley - Implemented TrustWallet, BraveWallet, and OkxWallet. In the process, made some good refactors to connectWallet / login flow (specifically the way I'm checking if an account exists prior to logging in, adding an account, or signing up) as well as refactored the API pattern I was using to get these accounts from backend (using GETs instead of POSTs per @hawyar's feedback).

Other wallets I am looking into include Binance, Jaxx Liberty, My Ether Wallet, Exodus Wallet, and Atomic Wallet, but I think this is a good start for now and can circle back if we declare it a priority to add any or all of these wallets.

As an aside, I noticed OkxWallet does not play well with other browser wallet extensions. It seems to overwrite the window.ethereum object that is critical for the other browser wallet to function. This isn't something that's necessarily within our control and is not something users would experience uniquely within our app (i.e., if they're using OkxWallet, they're probably not using other browser providers because of this issue). We can still offer OkxWallet to the users who are using this and it may not cause an issue unique to our app, so perhaps there is nothing to fix here (for now).

@ccali11 ccali11 requested a review from shanejearley May 19, 2023 19:09
signEthersMessage,
sendEthersTransaction,
switchEthersNetwork
}
}

function getBrowserProviders(ethereum: any) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactoring this appears to have resolved the issue I was having (and Lido still appears to have) where attempting to open MetaMask would open CoinbaseWallet instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dealing with multiple libraries that pollute the window namespace is difficult. I share your frustration. What if we created our own object inside window which would include all providers, something like window.__casimirProviders. Then each time we load a new provider we would copy its content from window.ethereum to window.__casimirProviders.etheruem and delete it from window. This would require us to load the libraries in a different way than we are doing now.

window.__casimirProviders = {
    provider1: okx,
    provider2: ethereum,
    provider3: anotherone
}

@ccali11 ccali11 requested a review from hawyar May 22, 2023 15:25
apps/web/src/composables/ethers.ts Outdated Show resolved Hide resolved
apps/web/src/composables/users.ts Show resolved Hide resolved
common/data/src/schemas/user.schema.json Show resolved Hide resolved
signEthersMessage,
sendEthersTransaction,
switchEthersNetwork
}
}

function getBrowserProviders(ethereum: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Dealing with multiple libraries that pollute the window namespace is difficult. I share your frustration. What if we created our own object inside window which would include all providers, something like window.__casimirProviders. Then each time we load a new provider we would copy its content from window.ethereum to window.__casimirProviders.etheruem and delete it from window. This would require us to load the libraries in a different way than we are doing now.

window.__casimirProviders = {
    provider1: okx,
    provider2: ethereum,
    provider3: anotherone
}

@ccali11
Copy link
Contributor Author

ccali11 commented May 22, 2023

Merging this in and closing it out. I will look into creating and injecting our own object in window instead of relying on the browser wallet's api which appear to be conflicting with each other.

@ccali11 ccali11 merged commit 7d69d81 into develop May 22, 2023
@ccali11 ccali11 deleted the feature/more-wallets branch May 22, 2023 18:52
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.

None yet

2 participants