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

fix: Filter out ALL_PERMISSIONS from keys in LSP6_DEFAULT_PERMISSIONS. #409

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

richtera
Copy link
Collaborator

@richtera richtera commented Apr 4, 2024

What kind of change does this PR introduce (bug fix, feature, docs update, ...)?

Filter out keys from LSP6_DEFAULT_PERMISSIONS because it now has ALL_PERMISSIONS.
Having ALL_PERMISSIONS set during encodePermissions will always return ALL_PERMISSIONS.
Currently calling decodePermissions will always return ALL_PERMISSIONS on.

What is the current behaviour (you can also link to an open issue here)?

What is the new behaviour (if this is a feature change)?

Other information:

Copy link
Collaborator

@CJ42 CJ42 left a comment

Choose a reason for hiding this comment

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

can we remove biome from this PR, and put it in a separate PR please?
Otherwise the PR is really hard to review as there are too many changes and I can't filter out what is related to the permissions

.vscode/settings.json Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@richtera richtera left a comment

Choose a reason for hiding this comment

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

Since there are a lot of style changes here are the real changes.

src/lib/permissions.ts Outdated Show resolved Hide resolved
src/lib/permissions.ts Outdated Show resolved Hide resolved
src/index.test.ts Show resolved Hide resolved
src/index.test.ts Show resolved Hide resolved
@richtera
Copy link
Collaborator Author

richtera commented Apr 6, 2024

I removed import sorting, so now only the cleanup and fixes are there.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2024

Codecov Report

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

Project coverage is 81.89%. Comparing base (9640d9f) to head (f52f1cf).
Report is 165 commits behind head on develop.

Files Patch % Lines
src/index.ts 70.00% 5 Missing and 1 partial ⚠️
src/lib/decodeData.ts 83.33% 0 Missing and 1 partial ⚠️
src/lib/encoder.ts 88.88% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #409      +/-   ##
===========================================
- Coverage    83.71%   81.89%   -1.83%     
===========================================
  Files           18       22       +4     
  Lines         1130     1364     +234     
  Branches       255      310      +55     
===========================================
+ Hits           946     1117     +171     
- Misses          98      138      +40     
- Partials        86      109      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@richtera richtera force-pushed the fix/repair-all-permissions branch 2 times, most recently from 5ea6e3a to 24bda3a Compare April 6, 2024 16:40
Copy link
Collaborator

@CJ42 CJ42 left a comment

Choose a reason for hiding this comment

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

Added some review comments. Otherwise some nice cleanups here! 🙌

package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
src/index.test.ts Show resolved Hide resolved
src/index.test.ts Show resolved Hide resolved
src/lib/permissions.ts Show resolved Hide resolved
src/index.test.ts Outdated Show resolved Hide resolved
src/index.test.ts Show resolved Hide resolved
@CJ42 CJ42 merged commit e5f34c7 into develop Apr 11, 2024
2 checks passed
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.

4 participants