-
Notifications
You must be signed in to change notification settings - Fork 105
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
RecordsPermissionScope Revocation must be scoped to a protocol #754
RecordsPermissionScope Revocation must be scoped to a protocol #754
Conversation
f03849f
to
d51bb62
Compare
f678d9f
to
726c44f
Compare
4e911a2
to
93ef1b9
Compare
726c44f
to
b995fe9
Compare
8c88313
to
45ab0dd
Compare
b995fe9
to
955da16
Compare
955da16
to
ab80f65
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #754 +/- ##
==========================================
+ Coverage 98.55% 98.57% +0.01%
==========================================
Files 73 73
Lines 10929 10986 +57
Branches 1573 1582 +9
==========================================
+ Hits 10771 10829 +58
+ Misses 152 151 -1
Partials 6 6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Legit! And thanks for breaking up the PRs! Really makes reviewing a much pleasant and efficient experience.
Happy to chat if you'd like!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐐 🚀
Optional nit:
Do you think passing the message JSON directly for revocation creation is easier as dev-ex?
Non-blocking, code can be merged as is, and can be done later even if you agree.
Upon reviewing your code, I realized some of the typing isn't ideal (my code) and I probably meant to fix it but just forgot. A small PR will quickly follow this one it's merged.
Yeah I was debating this, but wasn't sure which object the user would have more readily available so i opted with the parsed type. I'll merge this in and we can improve it a bit in a subsequent PR. 🙏 |
In order to get Permission Revocation messages associated with a specific protocol from the
MessageStore
it is necessary for the revocation messages to be tagged with theprotocol
from the grant scope.In this PR:
protocol
property to thePermissionRevocationCreateOptions
which normalizes and adds the provided protocol as a tag.preProcessingForCoreRecordsWrite
method to theRecordsWriteHandler
where if the incoming message is a revocation message, the original grant will be fetched and the protocol tag from the revocation will be compared to the grant scope protocol.