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

feat: events for ibc transfers #4874

Merged
merged 2 commits into from
Oct 2, 2024
Merged

feat: events for ibc transfers #4874

merged 2 commits into from
Oct 2, 2024

Conversation

cronokirby
Copy link
Contributor

@cronokirby cronokirby commented Oct 1, 2024

Describe your changes

This adds specific events related to fungible token transfers, recording them as they happen in the shielded pool.

First of all, this is a much more legible and readily consumable event around actual IBC events we care about, which is reason enough to have this, imo, but also, there's currently a flaw in the whole event system in that we don't have access, through events only, to the actual acknowledgement data for a packet.

In particular, we can't tell using the raw ibc events if a packet was acked successfully, finalizing a transfer, or unsuccessfully, causing the transfer to be refunded.
Knowing which of the two matters a lot, and will cause queries like "how much of this asset has been locked in the shielded pool" to not return the right result by quite a bit.

Checklist before requesting a review

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

    Event addition only

This adds events for outbound, inbound, and refunds.
This also adds sufficient metadata to uniquely identify which transfer
is being refunded, and where transfers are going.
Copy link
Contributor

@aubrika aubrika left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cronokirby cronokirby mentioned this pull request Oct 1, 2024
1 task
@cronokirby
Copy link
Contributor Author

Note: there is a bug in this and the related PR for refunds, where the receiver address for refunds is populated incorrectly.

@conorsch
Copy link
Contributor

conorsch commented Oct 2, 2024

Note: there is a bug in this and the related PR for refunds

Fixed on latest push, confirm?

@conorsch
Copy link
Contributor

conorsch commented Oct 2, 2024

Confirmed with @cronokirby out of band that this patch is good to go.

@conorsch
Copy link
Contributor

conorsch commented Oct 2, 2024

Refs #4871

@conorsch conorsch changed the title Transfer events feat: events for ibc transfers Oct 2, 2024
@conorsch conorsch merged commit e16700f into main Oct 2, 2024
14 checks passed
@conorsch conorsch deleted the transfer-events branch October 2, 2024 21:18
@conorsch conorsch mentioned this pull request Oct 4, 2024
7 tasks
conorsch pushed a commit that referenced this pull request Oct 7, 2024
## Describe your changes

This implements indexing for IBC transfer events.

Merge #4874 first.


## Checklist before requesting a review

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

  > indexing only
conorsch pushed a commit that referenced this pull request Oct 7, 2024
A backport of #4874 to v0.79.X, for reindexing purposes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants