swapr-dapp
swapr-dapp copied to clipboard
SwaprHQ
Add Tally wallet
https://tally.cash/ Tally is a community-owned and operated wallet.
Please let me know if I missed anything.
Tested locally:
- with no wallet extensions installed
- with only MetaMask installed
- with only Tally installed
- with both MetaMask and Tally installed (including toggling Tally's 'Tally as default' option)
Just realized I didn't select the develop branch. I will fix this tomorrow.
Thank you for switching the target branch for me. I re-tested to confirm behavior is still as expected on the develop branch.
I see that the check failure is unrelated to these changes. Please let me know if anything further is needed from my end.
We haven’t had the bandwidth to review this. I’ll see if we can review and merge this in the next sprint.
Adam
On Tue 15. Feb 2022 at 6:23 PM beemeeupnow @.***> wrote:
I see that the check failure is unrelated to these changes. Please let me know if anything further is needed from my end.
— Reply to this email directly, view it on GitHub https://github.com/levelkdev/dxswap-dapp/pull/652#issuecomment-1040559217, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOWISGB7UIUNTPLFYLUOIDU3KDZJANCNFSM5OKIKYOA . You are receiving this because you are subscribed to this thread.Message ID: @.***>
-- Adam Azad Skickat från mobil
Test failed When using Tally wallet there is no way to change network from Ethereum to other, clicking on for ex. Rinkeby causes no action at all.
Test failed When using Tally wallet there is no way to change network from Ethereum to other, clicking on for ex. Rinkeby causes no action at all.
Multi-network is coming to Tally.
Should the dApp have code added to show when the requested change doesn't happen? (in a new PR?)
Hello,
Please let me know about my last question when you get a chance.
Thank you
Hey, not sure how it should look. Maybe there should be some notification to user? @0xVenky what you think about that?
Hmm im not sure how to replicate this but it seems that browser is not recognising I have Tally installed?
https://user-images.githubusercontent.com/33226956/157860488-81688fc9-5095-4e3b-ae6f-b35b6dc38c9d.mov
Also when Im in chrome Tally is available as option and when I click on It metmask prompt pops out...Is this just me or?
https://user-images.githubusercontent.com/33226956/157861556-f514712d-c846-4906-a342-fb8724d7e1e3.mov
@Mi-Lan There are multiple problems when you have multiple wallet installed, not only on this PR.
@Mi-Lan There are multiple problems when you have multiple wallet installed, not only on this PR.
Aha but I have only tally on firefox and I cant get it out of that state where it asks me to install it...
@Mi-Lan Have u got "Use as default" set to true in Tally? Because as i see when it is set to false, even when no other wallet is installed, it asks me to install it, but when i switch it to true it starts working correctly
I've also made some test and got some thoughts. Tested on Chrome and Firefox, works the same.
-
Metamask is not installed, but Tally is installed - app still asking for installing both wallets.
-
If Tally is NOT selected as default can't be selected (first scr). When IS selected as default wallet MetaMask is not visible (second scr).
-
When initializing connection popup has not adjusted text, it's showing connector's name.
-
Switching network doesn't work, as was mentioned above by @beemeeupnow . So would be great if the Tally would be able to show notification that multi-network is not supported yet.
Deploy Preview for swapr ready!
| Name | Link |
|---|---|
| Latest commit | d63e6f0a0d479eeb9b29054cc35154b18e7164e3 |
| Latest deploy log | https://app.netlify.com/sites/swapr/deploys/629f719c495bc80008649b4c |
| Deploy Preview | https://deploy-preview-652--swapr.netlify.app/ |
| Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hey, after some tests it looks good but there is some minor issue - when user is connected to metamask and switch default one to tally the tally is displayed as connected
If it hasn't easy fix we shouldnt focus on this imo
Tested and if Tally Wallet is not installed then I don’t get the option to install, I get Tally as an option in connect wallet only if already installed.
Hey, after some tests it looks good but there is some minor issue - when user is connected to metamask and switch default one to tally the tally is displayed as connected
Good find! It's not specific to Tally Ho wallet or any of the changes I have made.
This is also the behavior if you have both metamask and coinbase wallet installed in the current develop branch without the changes from this PR. (e.g. I connect with metamask, then disable the metamask extension and enable coinbase wallet extension, I see metamask when clicking 'Change wallet')
It would be good to fix, but is a general issue outside of this PR scope.
Is there anything holding back approvals at this time?
This is also the behavior if you have both metamask and coinbase wallet installed in the current develop branch without the changes from this PR. (e.g. I connect with metamask, then disable the metamask extension and enable coinbase wallet extension, I see metamask when clicking 'Change wallet')
That's what I'm working on right now. I've found the solution, but still in some cases it's buggy, especially using Brave browser. After Tally is merged I will test it out together. That's why I mentioned previously that adding window.tally might not be the best solution.
Is there anything holding back approvals at this time?
Not sure if Brave browser is supported, but doesn't work. Since Brave wallet pretends to be MetaMask I've found some cases, scrs below. Do you have plans to fix that?
Also getting too many logs. Are they necessery?
@karczuRF I pulled the latest and did encounter the same thing in Brave (Chrome was fine for me)
When selecting either option, Tally popped up for me, which I see as acceptable.
I'll have to look more into how Brave is adding 'isMetaMask' here, but don't feel it should hold up this PR.
@karczuRF I pulled the latest and did encounter the same thing in Brave (Chrome was fine for me)
When selecting either option, Tally popped up for me, which I see as acceptable.
I'll have to look more into how Brave is adding 'isMetaMask' here, but don't feel it should hold up this PR.
Alright I can agree that Brave is bigger issue and shouldn't hold up this PR, but unfortunately I still can see the problems which I've mentioned before. Eg using chrome with Tally disabled still doesn't work fine in my case. @niemam29 @MilanVojnovic95 can you check this out, please?
And also still too many logs are consoled.
@karczuRF I pulled the latest and did encounter the same thing in Brave (Chrome was fine for me) When selecting either option, Tally popped up for me, which I see as acceptable. I'll have to look more into how Brave is adding 'isMetaMask' here, but don't feel it should hold up this PR.
Alright I can agree that Brave is bigger issue and shouldn't hold up this PR, but unfortunately I still can see the problems which I've mentioned before. Eg using chrome with Tally disabled still doesn't work fine in my case. @niemam29 @MilanVojnovic95 can you check this out, please?
And also still too many logs are consoled.
For me this problem occurs only when i have installed metamask, tally and coinbase wallet. When i'm using only tally and metamask it is good
synced with latest develop branch I see your other PR focused on improving the injected behavior in general
Hey, We are fans of Tally wallet but I guess at its current state of this PR, we cant merge this version. Reasons listed below:
- Tally is only on Mainnet and Swapr has extremely little presence in Mainnet. When Tally wallet starts supporting multichains and supports chains that have Swapr contracts deployed, it would be good to create a new PR with the latest version of Tally wallet.
- In the current version, if user installs Tally wallet, Metamask remains hidden as the injected wallet assumes Tally wallet to be the default wallet. This will not be ideal for our userbase as predominantly most of them are Metamask.
We will keep an eye out on the future versions of Tally wallet and prioritise when its available on Arbitrum or Gnosis chain. :)
