fix: distinguish explicit vs implicit delegation permission revocation for Intents #2661
Conversation
| /// Block number when the permission was/will be EFFECTIVELY revoked, taking into consideration the overall provider delegation revocation. | ||
| pub revoked_at: BlockNumber, | ||
| /// Block number the permission was/will be explicitly revoked (i.e., not implicitly revoked by a top-level revocation) (0 = not revoked) | ||
| pub explicit_revoked_at: BlockNumber, |
There was a problem hiding this comment.
Opted to add new explicit_revoked_at instead of effective_revoked_at, because revoked_at in this context has historically ALWAYS meant the effective revocation block.
github-actions
bot
added
the
metadata-changed
github-actions
bot
removed
the
metadata-changed
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
github-actions
bot
added
the
metadata-changed
github-actions
bot
added
metadata-changed
shannonwells
left a comment
shannonwells
left a comment
There was a problem hiding this comment.
Looks good, ship it!
aramikm
left a comment
aramikm
left a comment
There was a problem hiding this comment.
Approving, added a few comment just to make sure.
| s.serialize_field("schema_id", &self.granted_id)?; | ||
| s.serialize_field("granted_id", &self.granted_id)?; | ||
| s.serialize_field("revoked_at", &self.revoked_at)?; | ||
| s.serialize_field("explicit_revoked_at", &self.explicit_revoked_at)?; |
There was a problem hiding this comment.
for these do we need to implement deserialization also? Since the default would not work
There was a problem hiding this comment.
I don't think we ever deserialize...? The serialization is only for the node to generate the RPC JSON response for the custom RPC, I think.
| SchemaGrantResponse: { | ||
| schema_id: 'IntentId', | ||
| revoked_at: 'BlockNumber', | ||
| explicit_revoked_at: 'BlockNumber', |
There was a problem hiding this comment.
nit: Do we need this field?
There was a problem hiding this comment.
So, originally I thought that we had changed the behavior of the endpoint to tweak the revoked_at to show the effective revocation block, so @enddynayn suggested I instead keep revoked_at unmodified and add effective_revoked_at. However, on further investigation, we've ALWAYS tweaked the response of this endpoint to show the effective revocation block. So, we could just leave it alone. But that would mean the only way to determine if a permission was explicitly revoked would be to use a storage query, and we're trying to avoid those by giving clients appropriate runtime APIs. So I figured if we add explicit_revoked_at, then we never need to use a storage query.
I'm happy to remove if there's strong consensus against, though, since there isn't really a strong use case for needing the explicit_revoked_at block.
There was a problem hiding this comment.
I like the idea of getting everything that is needed using this. My only concern was about backwards compatibility and making sure that there is no change of meaning in a way that if some service is using these would get confused or break.
There was a problem hiding this comment.
If the backwards compatibility is ensured either by changing the using services or etc. then this should be fine.
Description
This PR does the following:
revoked_atblock number to the overallDelegationResponse, indicating the revocation of the overall Provider relationship, rather than individual Intent(s)explicit_revoked_atblock number toDelegationResponsefor each delegated Intent; the existingrevoked_atmaintains its original meaning of the earlier of the Intent or overall Delegation revocation block.Goal
Closes #2660
Checklist