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

Calculate channel value across all channels for native tokens #3219

Merged
merged 277 commits into from
Nov 7, 2022

Conversation

nicolaslara
Copy link
Contributor

The channel value for native tokens shouldn't be only the value in escrow for that channel, but the value for all channels. This is similar to what we were doing before: getting the total supply, but focuses on the IBC supply of native tokens.

@nicolaslara nicolaslara requested a review from a team November 2, 2022 18:24
Comment on lines 162 to 168
return bankKeeper.GetBalance(ctx, escrowAddress, denom).Amount
} else {
escrowAddress := transfertypes.GetEscrowAddress(port, channel)
return bankKeeper.GetBalance(ctx, escrowAddress, denom).Amount
// For native tokens, obtain the balance held in escrow for all potential channels
channels := channelKeeper.GetAllChannels(ctx)
balance := sdk.NewInt(0)
for _, channel := range channels {
escrowAddress := transfertypes.GetEscrowAddress("transfer", channel.ChannelId)
balance = balance.Add(bankKeeper.GetBalance(ctx, escrowAddress, denom).Amount)

}
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried about this being too slow for native assets right now.

Do we want to comment out for native assets until we can get this into a pre-computed state entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I was wondering about that too.

We need the value to be calculated across all channels if we want to be able to filter the tokens for any channel (like we discussed). I can add the pre-computed state and a query for it, but it's a detour.

We could also do the queries concurrently, but we still need to do one query per channel.

Copy link
Member

@ValarDragon ValarDragon Nov 2, 2022

Choose a reason for hiding this comment

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

Maybe we do native assets = bank supply, non-native = escrow account for now?

And then once we get an efficient state entry for the sum of IBC'd out native assets across all channels, we switch the go side? (Though actually, can we push all this logic to contract side?)

Copy link
Member

@ValarDragon ValarDragon Nov 2, 2022

Choose a reason for hiding this comment

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

Made this issue in IBC-go to track the more efficient version for this: cosmos/ibc-go#2664

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me. I'll make the changes.

It's pretty much what we had before 😅, but should be easier to modify now since the tests are more flexible.

Copy link
Member

Choose a reason for hiding this comment

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

sorry :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha, no worries. Now we know what to do :)

@nicolaslara nicolaslara added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Nov 3, 2022
@mergify mergify bot merged commit 28b6bc1 into main Nov 7, 2022
@mergify mergify bot deleted the nicolas/ibc-rate-limit/channel-value-across-channels branch November 7, 2022 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants