search

cosmos / solidity-ibc-eureka

This is a work-in-progress solidity implementation of IBC Eureka.
MIT License
26 stars 2 forks source link

added events to new directory #62

Closed DrParadox05 closed 1 month ago

DrParadox05 commented 2 months ago

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.

DrParadox05 commented 2 months ago

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.

DrParadox05 commented 2 months ago

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.

crodriguezvega commented 2 months ago

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.

crodriguezvega commented 2 months ago

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.

DrParadox05 commented 1 month ago

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.

crodriguezvega commented 1 month ago

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.

sangier commented 1 month ago

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.

crodriguezvega commented 1 month ago

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.