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.

Sign up for GitHub

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

Jump to bottom

feat: verify v2 #5236

Merged
merged 28 commits into from
Aug 15, 2024
Merged

feat: verify v2 #5236

merged 28 commits into from
Aug 15, 2024

Conversation

ganchoradkov
Copy link
Member

@ganchoradkov ganchoradkov commented Aug 1, 2024 •
edited
Loading

Description

Implemented verify v2 according to spec https://www.notion.so/walletconnect/Verify-Server-rewrite-tech-doc-Verify-API-V2-30e207c77f8447d8af37a186799b7c9e?pvs=4#fbc0ca6d67c444118599ba49d8b358f9

Type of change

  • Chore (non-breaking change that addresses non-functional tasks, maintenance, or code quality improvements)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Draft PR (breaking/non-breaking change which needs more work for having a proper functionality [Mark this PR as ready to review only when completely ready])
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

reown-com/web-examples#662

Checklist

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Additional Information (Optional)

Please include any additional information that may be useful for the reviewer.

@arein arein added the accepted label Aug 1, 2024
ganchoradkov and others added 11 commits August 6, 2024 10:31
@ganchoradkov
refactor: uses getDocument()
f755ef6
@ganchoradkov
Merge branch 'v2.0' into feat/verify-v2
07823a5
@ganchoradkov
Merge branch 'feat/verify-v2' of github.com:WalletConnect/walletconne…
2c79c58 
…ct-monorepo into feat/verify-v2

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@ganchoradkov
refactor: makes verify v2 public key being fetched only when attempte…
318a0e2 
…d to be used and to fetch new public key if validation fails with the current stored key
@ganchoradkov
chore: canary relay api
d11901e
@ganchoradkov
chore: prettier
18d5f6b
@ganchoradkov
fix: adds new attestation field to publisher tests
6b5a70a
@ganchoradkov
fix: relayer test with new attestation param
e0d979e
@ganchoradkov
chore: updates relay-api to stable
730bca3
@ganchoradkov
feat: adds test
5210e5a
@ganchoradkov
chore: clean up logs
3ddbfc7
@ganchoradkov ganchoradkov marked this pull request as ready for review August 9, 2024 10:39
@ganchoradkov ganchoradkov mentioned this pull request Aug 9, 2024
Closed
1 task
@ganchoradkov ganchoradkov changed the title wip: feat: verify v2 feat: verify v2 Aug 9, 2024
@chris13524
Copy link
Member

chris13524 commented Aug 12, 2024 •
edited
Loading

There is a race condition that I'd like to see resolved, if not before merging then let's create an issue for it.

The logic is if there isn't a saved public key in the verification step then it will request the public key in a blocking manner. In the happy case the public key will already be requested and stored during SDK initialization, but if the network request isn't complete when the verification step comes, then it will result in a second request and additional latency.

The race condition can be avoided by awaiting the original request in the verification step.

[ key fetch in SDK init      ]
  [receive paring req ][ key fetch again ]

Cali93
Cali93 reviewed Aug 12, 2024
View reviewed changes
Copy link
Contributor

@Cali93 Cali93 left a comment

Choose a reason for hiding this comment

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

Looks solid 💪
A few more tests and it should be good to go

packages/core/src/controllers/verify.ts
} catch (e) {
this.logger.warn(e);
}
return "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we retruning an empty string ?

@ganchoradkov ganchoradkov requested a review from Cali93 August 15, 2024 08:57
Cali93
Cali93 approved these changes Aug 15, 2024
View reviewed changes
Copy link
Contributor

@Cali93 Cali93 left a comment

Choose a reason for hiding this comment

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

LFG 🚀

@ganchoradkov ganchoradkov merged commit ab408aa into v2.0 Aug 15, 2024
9 checks passed
@ganchoradkov ganchoradkov deleted the feat/verify-v2 branch August 15, 2024 10:18
@ganchoradkov ganchoradkov mentioned this pull request Aug 15, 2024
Merged
20 tasks
github-merge-queue bot referenced this pull request in valora-inc/wallet Aug 20, 2024
@renovate
chore(deps): update dependency @walletconnect/react-native-compat to …
4d5d700 
…^2.15.0 (#5795)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[@walletconnect/react-native-compat](https://togithub.com/walletconnect/walletconnect-monorepo)
| [`^2.14.0` ->
`^2.15.0`](https://renovatebot.com/diffs/npm/@walletconnect%2freact-native-compat/2.14.0/2.15.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@walletconnect%2freact-native-compat/2.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@walletconnect%2freact-native-compat/2.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@walletconnect%2freact-native-compat/2.14.0/2.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@walletconnect%2freact-native-compat/2.14.0/2.15.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>walletconnect/walletconnect-monorepo
(@&#8203;walletconnect/react-native-compat)</summary>

###
[`v2.15.0`](https://togithub.com/WalletConnect/walletconnect-monorepo/releases/tag/2.15.0)

[Compare
Source](https://togithub.com/walletconnect/walletconnect-monorepo/compare/2.14.0...2.15.0)

#### What's Changed

- chore: prep for `2.14.0` release by
[@&#8203;ganchoradkov](https://togithub.com/ganchoradkov) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5195](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5195)
- refactor: removes session approval peer acknowledgment delay by
[@&#8203;ganchoradkov](https://togithub.com/ganchoradkov) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5197](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5197)
- chore(deps): update docker/build-push-action action to v6 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/4641](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/4641)
- chore(deps): update actions/add-to-project action to v1.0.2 by
[@&#8203;renovate](https://togithub.com/renovate) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5101](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5101)
- chore: fix some comments by
[@&#8203;renshuncui](https://togithub.com/renshuncui) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/4640](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/4640)
- AlgorandProvider
[#&#8203;877](https://togithub.com/walletconnect/walletconnect-monorepo/issues/877)
[#&#8203;2760](https://togithub.com/walletconnect/walletconnect-monorepo/issues/2760)
by [@&#8203;scholtz](https://togithub.com/scholtz) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5201](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5201)
- chore: fix typo by
[@&#8203;riyueguang](https://togithub.com/riyueguang) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5216](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5216)
- chore: change handshake canary to only measure 1 client initialization
by [@&#8203;chris13524](https://togithub.com/chris13524) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5250](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5250)
- refactor: session settle response by
[@&#8203;ganchoradkov](https://togithub.com/ganchoradkov) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5259](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5259)
- refactor: publishes session settle request before the session propose…
by [@&#8203;ganchoradkov](https://togithub.com/ganchoradkov) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5246](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5246)
- feat: verify v2 by
[@&#8203;ganchoradkov](https://togithub.com/ganchoradkov) in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5236](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5236)
- chore: partially dedupe fetch implementations and use native fetch
instead by [@&#8203;talentlessguy](https://togithub.com/talentlessguy)
in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5112](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5112)

#### New Contributors

- [@&#8203;renshuncui](https://togithub.com/renshuncui) made their first
contribution in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/4640](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/4640)
- [@&#8203;scholtz](https://togithub.com/scholtz) made their first
contribution in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5201](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5201)
- [@&#8203;riyueguang](https://togithub.com/riyueguang) made their first
contribution in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5216](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5216)
- [@&#8203;talentlessguy](https://togithub.com/talentlessguy) made their
first contribution in
[https://github.com/WalletConnect/walletconnect-monorepo/pull/5112](https://togithub.com/WalletConnect/walletconnect-monorepo/pull/5112)

**Full Changelog**:
WalletConnect/walletconnect-monorepo@2.14.0...2.15.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 5pm,every weekend" in timezone
America/Los_Angeles, Automerge - "after 5pm,every weekend" in timezone
America/Los_Angeles.

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job
log](https://developer.mend.io/github/valora-inc/wallet).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsibnBtIiwicmVub3ZhdGUiXX0=-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: valora-bot <valorabot@valoraapp.com>
@ganchoradkov ganchoradkov restored the feat/verify-v2 branch August 20, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris13524 chris13524 chris13524 approved these changes

@Cali93 Cali93 Cali93 approved these changes

@glitch-txs glitch-txs Awaiting requested review from glitch-txs

@jakubuid jakubuid Awaiting requested review from jakubuid

@bkrem bkrem Awaiting requested review from bkrem

@ignaciosantise ignaciosantise Awaiting requested review from ignaciosantise

Assignees
No one assigned
Labels
accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ganchoradkov @chris13524 @ignaciosantise @Cali93 @arein