Conversation
bkrem
left a comment
bkrem
left a comment
There was a problem hiding this comment.
Submitting first pass notes. Will do final run through also on the added code in engine shortly
| senderPublicKey: selfPublicKey, | ||
| receiverPublicKey: dappPublicKey, | ||
| } | ||
| ).then((id) => { |
There was a problem hiding this comment.
What happens if this throws? Assuming it will still throw the whole subscribe fn as it did before with async/await?
Nit: would prefer to keep the async/await style consistent that's already used throughout the fn instead of switching to chain syntax right at the end.
There was a problem hiding this comment.
Honestly I just know it's not good practice to use async () => to create a promise and I needed a promise to arbitrarily resolve. I'll add a catch
There was a problem hiding this comment.
Fair point, hence only a nit :) Assuming this throws as it did before no need for a catch I guess since the behaviour should just stay the same if possible
| ); | ||
|
|
||
| const claims = | ||
| this.decodeAndValidateJwtAuth<NotifyClientTypes.UpdateResponseJWTClaims>( |
There was a problem hiding this comment.
Not key for this PR, but this section and the one for notify_subscription_response are basically identical apart from the RPC method from what I can tell.
Would be good to refactor this to be abstracted later, to remove the possibility of bugs when updating one flow and not the other by mistake.
There was a problem hiding this comment.
Absolutely - i had the same thought after Los Alamos but I never found the time
|
Looking solid and excellent PR context 🙏 |
Co-authored-by: Ben Kremer <ben@walletconnect.com>
devceline
force-pushed
the
feat/use-sbs
branch
from
1eb0b4a to
f996a63
Compare
2 participants
Note
Only merging after a new version of core is released with changes from canaryChanges fix the reason the CI was flaky sometimespending_sub_watch_labelwas not unique per subscriptionChanges
subscribe,delete, andupdatesbsproperty coming from notify server in the responses for the above functionsnotify_subscriptions_changed(not all, more to come later)sbsfrom spec: https://github.com/WalletConnect/walletconnect-specs/pull/182/filesWarning
We are not setting the
mjvflag as we still wantnotify_subscriptions_changedfor now, to maintain complete compatibility with web3inbox. Will refactor when we have more timeSpec TL;DR
Add
sbsflag tosubscribe,deleteandupdatefunctions. This includes latest state of subscription, causing us to not need to depend onnotify_subscriptions_changedfor latest state. Furthermore, functions only resolve after latest state is synced to local state.Reminder of architecture: