Feature/pr 3945 account permission #30
Conversation
elffjs
left a comment
elffjs
left a comment
There was a problem hiding this comment.
My only serious comment is about the event PermissionsSet. The rest are nitpicks.
contracts/Sacd.sol
Outdated
| templateId | ||
| ); | ||
|
|
||
| emit PermissionsSet(address(0), 0, permissions, grantee, expiration, templateId, source); |
There was a problem hiding this comment.
From the perspective of an event listener: I think we're losing msg.sender (the "grantor") here. We could use that instead of address(0) in the event arguments, although it seems strange to me to do a new function but not a new event. What do you think?
There was a problem hiding this comment.
Yeah. My main concern was to keep the same event to not break anything. Now that I found out the asset is the same as the grantor in other places, makes sense to add that
contracts/Sacd.sol
Outdated
| return false; | ||
| } | ||
|
|
||
| return (pr.permissions & permissions) == permissions; |
There was a problem hiding this comment.
It seems like hasAccountPermission and getAccountPermissions share a lot of code. Any opportunity for consolidation?
contracts/Sacd.sol
Outdated
| if (!_isTemplateActive(pr.templateId)) { | ||
| return false; | ||
| if (_isPermissionValid(pr.expiration, $.templateContract, pr.templateId)) { | ||
| return (pr.permissions >> (2 * permissionIndex)) & 3 == 3; |
There was a problem hiding this comment.
My feeling was that you could call hasPermissions with 3 << (2 * permissionIndex). Am I wrong? Does it cost more gas?
Total code golf at this point, don't worry about it too much.
There was a problem hiding this comment.
Any extra function call costs more gas, but it is quite cheap. And I think this is nice and clean solution
No description provided.