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.

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

build(deps): upgrade wallet connect dependencies #5129

Merged
merged 10 commits into from
Mar 25, 2024

Conversation

MuckT
Copy link
Collaborator

@MuckT MuckT commented Mar 19, 2024

Description

Upgrades Wallet Connect dependencies to the latest versions: originally pinned in #4406.

Test plan

  • Tested locally on iOS and Android
    • Connect
    • Verify
    • Sign Transaction

Related issues

Backwards compatibility

Yes

Network scalability

N/A

Copy link

emerge-tools bot commented Mar 20, 2024

1 build increased size

Name Version Download Change Install Change Approval
Celo (test)
org.celo.mobile.test
1.81.0 (146) 24.2 MB ⬆️ 16.6 kB (0.07%) 60.4 MB ⬆️ 12.4 kB (0.02%) N/A

Celo (test) 1.81.0 (146)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 12.4 kB (0.02%)
Total download size change: ⬆️ 16.6 kB (0.07%)

Largest size changes

Item Install Size Change
🗑 celo ⬇️ -15.5 MB
📝 valora ⬆️ 1.6 MB
📝 Lottie ⬆️ 785.9 kB
📝 Code Signature ⬆️ 662.7 kB
📝 Strings.CFStrings ⬆️ 650.7 kB
View Treemap

Image of diff


🛸 Powered by Emerge Tools

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 85.70%. Comparing base (bf0b169) to head (04a4fbc).
Report is 7 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5129      +/-   ##
==========================================
- Coverage   85.70%   85.70%   -0.01%     
==========================================
  Files         729      730       +1     
  Lines       29854    29863       +9     
  Branches     5160     5156       -4     
==========================================
+ Hits        25587    25594       +7     
- Misses       4032     4034       +2     
  Partials      235      235              
Files Coverage Δ
src/redux/migrations.ts 97.32% <100.00%> (+<0.01%) ⬆️
src/redux/store.ts 78.68% <ø> (ø)
test/schemas.ts 90.00% <100.00%> (+0.03%) ⬆️
src/walletConnect/reducer.ts 21.95% <0.00%> (ø)

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf0b169...04a4fbc. Read the comment docs.

@MuckT MuckT marked this pull request as ready for review March 21, 2024 22:15
@MuckT
Copy link
Collaborator Author

MuckT commented Mar 22, 2024

Not sure why Codecov is complaining. The new type is tested in src/walletConnect/screens/SessionRequest.test.tsx.

package.json Outdated
"@walletconnect/sign-client": "2.10.0",
"@walletconnect/types": "2.10.0",
"@walletconnect/sign-client": "2.11.3",
"@walletconnect/types": "2.11.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we still need to keep these as exact versions instead of range versions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in c1c69ff!

@@ -87,7 +87,7 @@ export const reducer = (
.includes(pendingAction.topic)
),
pendingSessions: state.pendingSessions.filter(
(pendingSession) => pendingSession.params.expiry > action.dateInSeconds
(pendingSession) => pendingSession.params.expiryTimestamp > action.dateInSeconds
Copy link
Contributor

Choose a reason for hiding this comment

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

@MuckT codecov is complaining about this line not having tests. Maybe worth adding one given this is a more complex reducer?
Also, would this need a migration? could there be a persisted pendingSession with expiry instead of expiryTimestamp?

Copy link
Collaborator Author

@MuckT MuckT Mar 23, 2024

Choose a reason for hiding this comment

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

I thought pending sessions where pretty ephemeral, but at second glance we might need migration otherwise the old ones may never expire. I wonder if it might be safer and easier to clear the pendingSessions entirely instead of a migration the pendingSessions params.expiry to params.expiryTimestamp.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleared pending sessions with a migration in 057244a and tested in 04a4fbc.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the only change in schema is expiry to expiryTimestamp, then an inplace migration is fine, but if other fields are also changing, clearing seems good. Also, just for my understanding, how do pending sessions work when the app restarts?

@@ -87,7 +87,7 @@ export const reducer = (
.includes(pendingAction.topic)
),
pendingSessions: state.pendingSessions.filter(
(pendingSession) => pendingSession.params.expiry > action.dateInSeconds
(pendingSession) => pendingSession.params.expiryTimestamp > action.dateInSeconds
Copy link
Contributor

Choose a reason for hiding this comment

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

If the only change in schema is expiry to expiryTimestamp, then an inplace migration is fine, but if other fields are also changing, clearing seems good. Also, just for my understanding, how do pending sessions work when the app restarts?

@MuckT
Copy link
Collaborator Author

MuckT commented Mar 25, 2024

@satish-ravi pending sessions are added whenever there is a SESSION_PROPOSAL action and are removed whenever this proposal is accepted or denied. In the case of an application close while there are pending session it should be served when the application is reopened.

@MuckT MuckT added this pull request to the merge queue Mar 25, 2024
Merged via the queue into main with commit bc9afe0 Mar 25, 2024
16 checks passed
@MuckT MuckT deleted the tomm/update-wc-dependencies branch March 25, 2024 23:53
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
### Description

Upgrades Wallet Connect dependencies to the latest versions: originally
pinned in valora-inc#4406.

### Test plan

- Tested locally on iOS and Android
  - Connect
  - Verify
  - Sign Transaction

### Related issues

- Related: ACT-962

### Backwards compatibility

Yes

### Network scalability

N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants