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: Update @safe-global/safe-apps-sdk to ^8.0.0 #2339

Merged
merged 4 commits into from
Aug 14, 2023
Merged

Feat: Update @safe-global/safe-apps-sdk to ^8.0.0 #2339

merged 4 commits into from
Aug 14, 2023

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Aug 1, 2023 •
edited
Loading

What it solves

Resolves #2337

How this PR fixes it

By updating the safe apps SDK to the latest version. I also had to fix a couple of linter errors and add TextEncoder to the global env in jest because it was required by viem but not included with JSDom.

How to test it

Open a test safe app, and try to execute a couple of transactions and message signings.

Screenshots

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

@mmv08 mmv08 requested review from schmanu and iamacook August 1, 2023 09:35
@github-actions
Copy link

github-actions bot commented Aug 1, 2023 •
edited
Loading

Branch preview

✅ Deploy successful!

https://feat_update_safe_apps_sdk--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Aug 1, 2023 •
edited
Loading

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

schmanu
schmanu previously requested changes Aug 1, 2023
View reviewed changes
Copy link
Member

@schmanu schmanu left a comment

Choose a reason for hiding this comment

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

Something is not right.
The deployment failed and all unit tests that import from the safe-apps-sdk fail.

@mmv08 mmv08 marked this pull request as draft August 1, 2023 11:08
@mmv08 mmv08 force-pushed the feat/update-safe-apps-sdk branch from 6ac822c to 7e4e943 Compare August 1, 2023 11:24
mmv08
mmv08 commented Aug 1, 2023
View reviewed changes
src/components/safe-apps/types.ts
@@ -40,7 +40,7 @@ const FEATURES = [
'speaker-selection',
]

export type AllowedFeatures = typeof FEATURES[number]
export type AllowedFeatures = (typeof FEATURES)[number]
Copy link
Member Author

Choose a reason for hiding this comment

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

This threw a linter error without the braces

src/services/pairing/module.ts
Comment on lines +59 to +63
public emit: (typeof EventEmitter)['emit']
// @ts-expect-error - 'on' does not exist on `typeof EventEmitter`
public on: typeof EventEmitter['on']
public on: (typeof EventEmitter)['on']
// @ts-expect-error - 'removeListener' does not exist on `typeof EventEmitter`
public removeListener: typeof EventEmitter['removeListener']
public removeListener: (typeof EventEmitter)['removeListener']
Copy link
Member Author

Choose a reason for hiding this comment

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

This threw a linter error without the braces

@mmv08 mmv08 requested a review from schmanu August 1, 2023 11:36
@mmv08 mmv08 marked this pull request as ready for review August 1, 2023 11:36
@katspaugh
Copy link
Member

@mmv08 please solve the git conflicts, I've moved it to QA in the meanwhile.

iamacook
iamacook approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

There's a merge conflict.

@mmv08
Copy link
Member Author

mmv08 commented Aug 3, 2023

sorted

@mmv08 mmv08 force-pushed the feat/update-safe-apps-sdk branch from 2c0f831 to ff15ac3 Compare August 3, 2023 08:26
@mmv08
Copy link
Member Author

mmv08 commented Aug 3, 2023

The same tests fail in all the other PRs, so it isn't affected by my changes. Correct?

@katspaugh
Copy link
Member

Yes, the tests have unfortunately been flaky recently because of Goerli (supposedly).

@katspaugh katspaugh dismissed schmanu’s stale review August 4, 2023 11:48

It's all good now with the CI.

@francovenica
Copy link
Contributor

Works for me.
Note: A lot of the apps don't work in test envs.
I tried with Cow swap, Aave and a test app we use to generate custom messages. Still it would be great if we can ask the person who reported it to give it a try again since idk what app can requiere this "undefined" value

@mmv08
Copy link
Member Author

mmv08 commented Aug 7, 2023

I've realised that the undefined issue might be something different - in the transaction details parsing from the safe apps SDK message on the wallet side. But it is still worth updating the safe-apps-sdk to the latest version as it should improve the bundle size

@mmv08
Copy link
Member Author

mmv08 commented Aug 8, 2023

I'll leave up to you to merge this since I might be slightly unaware of the release processes/plans

@katspaugh
Copy link
Member

We'll include it in the release after the upcoming one if you don't mind.

@github-actions
Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total
Report generated by eslint-plus-action

@katspaugh katspaugh merged commit 17fd26c into dev Aug 14, 2023
7 checks passed
@katspaugh katspaugh deleted the feat/update-safe-apps-sdk branch August 14, 2023 13:24
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iamacook iamacook iamacook approved these changes

@schmanu schmanu schmanu approved these changes

@katspaugh katspaugh katspaugh approved these changes

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

Successfully merging this pull request may close these issues.

Update @safe-global/safe-apps-sdk to the latest version
5 participants
@mmv08 @katspaugh @francovenica @schmanu @iamacook