added events to new directory #62
Conversation
|
Hey @gjermundgaraba should I lint the code and update the PR? Also, I don't know golang so didn't test the e2e directory. P.S: I just remembered that the ICS20Transfer.json should be updated with the new event name. |
|
Hey @gjermundgaraba, in the latest commit I've ran linter and updated the ABI for ICS20Transfer.json. Wasn't able to test the e2e directory code as I'm not comfortable with golang. |
|
Hey @DrParadox05. Gjermund is on holidays now until end of next week, but I will review the PR and help you to (hopefully) get it merged. |
…nsfer event to previous name
|
The e2e tests run locally fine for me, so I think the failures are due to the fact that they are running from a fork and doesn't have access to the secrets. We will have to figure out a way to fix that, but for now I don't think we need to block the PR on that. |
There was a problem hiding this comment.
@DrParadox05 thanks for the effort!
I have to say I'm not big fan of having a specific interface for events only as it increases the complexity without bringing a real benefit. If this is the solution then I'd rather prefer to keep things as they are. E.g. I just checked that uniswap , and most of openzeppelin code use the same approach we already have in place, just defining events in the interfaces.
Other opinions?
|
Hey @sangier I agree. What should I do then? because I was told to move them in a different directory. We can close the issue and PR in that case. |
|
Thanks for the feedback @sangier. We could wait for @srdtrk or @gjermundgaraba feedback as well, but if keeping them where they are right now is the general best practices, then I am happy to follow that and close the issue and the PR. |
|
I mean, I see a value in dividing events and to specify them in specific folder when the project is really complex and, specifically, when you have events that can be generated by multiple distinct contracts (that have distinct interfaces). In that case you would use this approach to avoid code duplications, extending the contract interface with the eventInterface. In our concrete case, I don't see too much value in doing it. |
|
Then let's close the PR and the issue. Thank you for the time you spent on the work, @DrParadox05. Sorry that we will not merge it for now. |
Moved events to a new directory, renamed the event 'ICS20Transfer' to 'ICS20TokenTransfer' as it was colliding with the 'ICS20Transfer.sol' contract. ran unit tests as well.