-
Notifications
You must be signed in to change notification settings - Fork 541
FEATURE: Add Tally Ho Wallet Package #1238
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: Add Tally Ho Wallet Package #1238
Conversation
|
Thank you for this PR! We will give it a test early next week and get it merged in after successful test! |
|
Thanks @Adamj1232. Sorry about that, it was a remnant from testing. It has been removed. |
|
@0xzoz no problem at all, thank you! We will get it tested and merged early next week! |
|
Thanks for the feedback @Adamj1232. Ive addressed those issues. |
packages/sequence/src/index.ts
Outdated
| throw new Error('Failed to connect to the wallet') | ||
| } | ||
| } | ||
| return window?.ethereum && window.ethereum?.isSequence |
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.
You can revert the formatting within this file so you dont get tagged in the gitblame should any errors arise 😄
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.
uh oh... Done. There are also changes to the wallet connect package. Should I revert that as well?
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.
Lol no problem! Yeah if you could revert those that would be great and then I will go in and format so any issues point to me 🤓
After that is done I will pull down the branch and add the CI pipeline setup, test and we should be good to go.
Thank you so much!
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.
Great - reverted the changes. Always happy to have the issues not point to me lol(hard task). My pleasure!
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.
Excellent, I made a handful of changes if you want to pull. Ill run some final checks and testing and we should be ready to rock 🎸
|
Alrighty onto final stages of testing with Tally wallet! If you could ensure you have the latest develop merged into branch we should be good to go. |
confirmed 🐶 |
Excellent! I confirmed most behavior but I don't see Tally supporting any test nets so wasnt able to verify transactions. Have you verified on your end or am I missing a setting to allow testnets? |
You are correct. There is no testnet support at present(WIP). I usually test in prod with a small amount |
Adamj1232
left a comment
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.
Excellent work, thank you!
Description
Tally Ho is a community-built browser extension wallet. While web3-onboard currently provides Tally Ho as an injected option, it will only show the Tally Ho wallet if it is currently installed. As we ready for DAO launch as a small community-run project we need the ability to leverage awesome wallet connection libraries like web3-onboard to show Tally Ho and create brand awareness.
This PR sections out Tally Ho as a package that can be installed to allow Tally Ho to always show as a wallet option. If the wallet is detected as already installed, onboard will use the injected version. If it is not and a user clicks the Tally Ho button it will redirect them to the install page.
Checklist
package.jsonis incremented following semantic versioningyarn type-check&yarn buildto confirm there are not any associated errors