Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement spec events #108

Open
gjermundgaraba opened this issue Nov 18, 2024 · 6 comments · May be fixed by #126
Open

Implement spec events #108

gjermundgaraba opened this issue Nov 18, 2024 · 6 comments · May be fixed by #126
Assignees
Labels
enhancement Improvements

Comments

@gjermundgaraba
Copy link
Contributor

As soon as the IBC Eureka events are finalized (cosmos/ibc#1165) we need to do a full cleanup of the events we currently emit from our contracts.

We likely have some unnecessary events, so the initial plan here is to remove the current events and only implement the spec events. After that, we can decide if we need more events (and if those should also go into the spec).

@sangier
Copy link
Contributor

sangier commented Nov 26, 2024

hey @serdar @gjermund , note that if at this stage we won't consider support for async acks and the multipayload, then the events are already pretty much aligned with the spec and with serdar suggestion. The minimum required parameters are emitted.
What we could remove are the event emitted in the ics20Transfer, but this would result in a very little gain on gas cost. I would prefer to keep the event emission there.

I would suggest to icebox this till we decide to implement multipayload and/or async acks.

@srdtrk
Copy link
Member

srdtrk commented Nov 26, 2024

  1. Would multipayload only differ on ack events?
  2. Maybe you can open a PR to remove unnecessary events? (I think we use some of those events in the tests but maybe you can come up with another way to test the same logic)

@sangier
Copy link
Contributor

sangier commented Nov 26, 2024

  1. We would need to change sendPacket events too as in spec we emit a send_payload specific event in the multipayload case.
  2. Gonna check this out.

@gjermundgaraba
Copy link
Contributor Author

Did a quick check on removing emit ICS20Transfer(packetData, erc20Address); and it looks like it saves about 5k of gas to do so. I would say that we should remove anything that is not needed as we are trying to squeeze out as much performance as we can, and if this is not strictly needed, we should just get rid of it.

@sangier
Copy link
Contributor

sangier commented Nov 26, 2024

Ok, agreed. Will remove the transfer events so!

@srdtrk
Copy link
Member

srdtrk commented Nov 26, 2024

Let's also remove any other unnecessary events

@gjermundgaraba gjermundgaraba moved this from Ready for Development to In progress in IBC-GO Eureka Nov 26, 2024
This was referenced Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements
Projects
Status: In progress
3 participants