-
Notifications
You must be signed in to change notification settings - Fork 116
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
2958 eventsub #2960
2958 eventsub #2960
Conversation
A new deploy preview is available on Cloudflare Pages at https://79972cb6.taquito-test-dapp.pages.dev |
New packages have been deployed to the preview repository at https://npm.preview.tezostaquito.io/. Published packages:
|
Deploying taquito with Cloudflare Pages
|
@@ -59,19 +66,17 @@ export class BeaconWallet implements WalletProvider { | |||
} | |||
|
|||
async getPKH() { | |||
const account = await this.client.getActiveAccount(); | |||
if (!account) { | |||
if (!this.account) { |
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.
One concern:
I the dApp code calls getPkh immediately after connecting, but before the first ACTIVE_ACCOUNT_SET
event is received, they will get an error. Not sure if this is actually possible, or even if it's possible if there is a better solution.
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.
I don't think that will happen since the even will be subscribed to in the constructor. So getPkh will likely never be called unless the client has been constructed, and in consequence, the account
member variable will always be assigned a value. And if it doesn't it will throw an error, so at least it signals the user (or developer) that the wallet has not been connected, or the wallet info is incomplete. I hope that makes sense!
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.
While the event is subscribed in the constructor, it might not be triggered until a split-second later
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.
hmm, I figured it would be a rare occurrence. But do you have any suggestions to improve it then?
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.
It might be a good idea to investigate if it's possible to create the error mentioned in comments. If so, we might want to add something to release notes.
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.
Code change in packages/taquito-beacon-wallet/src/taquito-beacon-wallet.ts
looks good!! Might be pointing to master makes this pr regressed some commit changes already in master rn. Other than that just would be good to have test-dapp to switch account to prove the change to subscribeToEvent will capture switching account successfully in beaconWallet class.
* feat: updated beacon to 4.2.2-beta.0 * feat: feat: updated beacon to 4.2.2-beta.1 * feat: feat: updated beacon to 4.2.2-beta.2 * chore(releng) bump version to 19.2.1-beta.0 * feat: updated beacon to 4.2.2-beta.3 * feat: updating to private prc nodes * reverted config to use public testnets instead * fix: updating back to public node * chore(releng) bump version to 19.2.1-beta.1 * feat: updated beacon to 4.2.2-beta.4 * Revert "Merge branch 'master' of https://github.com/ecadlabs/taquito into beacon-4.2.2" This reverts commit caf851a, reversing changes made to ecd6f5a. * chore(releng) bump version to 19.2.1-beta.2 * 2958 eventsub (#2960) * updated taquito-beacon-wallet to use eventsub * removed comments * removed console log * trigger ci build * updated beacon version to v4.2.2 * chore(releng) bump version to 19.2.1 --------- Co-authored-by: Davis Sawali <davis.sawali@ecadlabs.com>
* updated taquito-beacon-wallet to use eventsub * removed comments * removed console log * trigger ci build
* feat: updated beacon to 4.2.2-beta.0 * feat: feat: updated beacon to 4.2.2-beta.1 * feat: feat: updated beacon to 4.2.2-beta.2 * chore(releng) bump version to 19.2.1-beta.0 * feat: updated beacon to 4.2.2-beta.3 * feat: updating to private prc nodes * reverted config to use public testnets instead * fix: updating back to public node * chore(releng) bump version to 19.2.1-beta.1 * feat: updated beacon to 4.2.2-beta.4 * chore(releng) bump version to 19.2.1-beta.2 * 2958 eventsub (#2960) * updated taquito-beacon-wallet to use eventsub * removed comments * removed console log * trigger ci build * updated beacon version to v4.2.2 * chore(releng) bump version to 19.2.1 --------- Co-authored-by: huianyang <hui-an.yang@ecadlabs.com> Co-authored-by: Davis Sawali <davis.sawali@ecadlabs.com>
closes #2958
Updated taquito beacon wallet to use
subscribeToEvent
instead ofgetActiveAccount
Thank you for your contribution to Taquito.
Before submitting this PR, please make sure:
In this PR, please also make sure:
closes #TICKETNUMBER
in the description box (when applicable)Release Note Draft Snippet
If relevant, please write a summary of your change that will be suitable for
inclusion in the Release Notes for the next Taquito release.