Add missing events for admin-related methods #1177
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
@@ Coverage Diff @@
## main #1177 +/- ##
==========================================
+ Coverage 57.53% 57.61% +0.07%
==========================================
Files 53 53
Lines 7289 7306 +17
==========================================
+ Hits 4194 4209 +15
- Misses 2800 2801 +1
- Partials 295 296 +1
|
There was a problem hiding this comment.
Thanks a lot for your PR! 👏
I had started some work yesterday already to speed this up but wasn't ready to push. You may want to 👀 at #1179 for inspiration or you review mine and we close this.
I would ask for tests in your PR. The coverage of setAdmin was not great before. I like to improve the coverage while working in an area. This helps with regression
| )) | ||
| } else { | ||
| ctx.EventManager().EmitEvent(sdk.NewEvent( | ||
| types.EventTypeClearContractAdmin, |
There was a problem hiding this comment.
I was thinking of 2 different events first but then merged them into 1 to make it easier for clients to subscribe to updates. When the admin is cleared, the new admin value would be empty.
There was a problem hiding this comment.
I like your option as well. I just thought about this and I decided to distinguish those events from the view of a message as a unit of action. (if the admin is cleared, nobody can update the admin after that and only gov can do this)
| types.EventTypeSetAccessConfig, | ||
| sdk.NewAttribute(types.AttributeKeyCodeID, strconv.FormatUint(codeID, 10)), | ||
| sdk.NewAttribute(types.AttributeKeyCodePermission, newConfig.Permission.String()), | ||
| sdk.NewAttribute(types.AttributeKeyAuthorizedAddresses, strings.Join(newConfig.Addresses, ", ")), |
There was a problem hiding this comment.
The newConfig.Address field is deprecated but we need to support this for some time, until fully removed.
The authorized addresses are not always set. I would prefer to set the attribute only for certain permissions where a value would be expected.
nit: the space in the join pattern makes it easier to read for humans but adds a bit more complexity for the clients. Either they find the pattern or have to trim each address.
There was a problem hiding this comment.
The authorized addresses are not always set. I would prefer to set the attribute only for certain permissions where a value would be expected.
I think it's fine to always add addresses attributes in schema perspective. The addresses field will just be left empty(Join nil), and the client can process the event data appropriately according to the CodePermission.
Thank you for your comment @alpe and i am fine whichever option you choose. If you wanna close this PR, I will review yours if it need it or I will add tests for this PR If this PR continue. |
|
I will close this PR now in favour of #1179 just to speed up the process for v0.31 since you had no strong opinion about the implementation. |
closes #1173