-
Notifications
You must be signed in to change notification settings - Fork 589
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
Calculate channel value across all channels for native tokens #3219
Conversation
This reverts commit 9ffdc37.
…las/ibc-rate-limit/new-denom-logic
x/ibc-rate-limit/rate_limit.go
Outdated
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) | ||
|
||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry :(
There was a problem hiding this comment.
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 :)
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.