Common interface for Package contract #140
Conversation
| @@ -85,7 +85,7 @@ contract App is Ownable { | |||
| function unsetPackage(string packageName) public onlyOwner { | |||
| require(address(providers[packageName].package) != address(0), "Package to unset not found"); | |||
| delete providers[packageName]; | |||
| emit PackageChanged(packageName, address(0), ""); | |||
| emit PackageChanged(packageName, address(0), [uint64(0), uint64(0), uint64(0)]); | |||
There was a problem hiding this comment.
What do you think about having sth like PackageSet and PackageUnset events instead?
There was a problem hiding this comment.
I think that having a single event is more useful in terms of watching the contract. If you are tracking the status of a particular package (or all packages), tracking only PackageSet events does not work, since you'd need the Unset as well. In other words, it wouldn't make sense to filter by Set events and not by Unset events at the same time; so it's just simpler for the client to merge them in a single event. OTOH, it feels odd to be filling args with empty values.
Anyway, I have no strong preference. I'd keep it as Changed just because it's the same pattern we're using in the ImplementationDirectory as well, and I don't see a reason to change that interface. But I'm ok with making the change if you think it's for the best.
Absolutely, will be working on that today |
spalladino
force-pushed
the
feature/common-package-interface-#138
branch
from
4d42757
to
318cc60
Compare
spalladino
removed
the
status:in-progress
|
@facuspagnuolo ready to review |
Fixes #138