Chore: Make a state entry to find total amount of native assets IBC'd out #3019

stackman27 · 2023-01-16T18:58:29Z

Description

Currently every channel has its own escrow bank account, which is a great design! So its very easy to see the amount of native assets IBC'd in or out on any IBC channel.

However, there are use cases that would like us to be able to reason about the total amount of a native asset that has been IBC'd out.

So if there are say three channels from Osmosis to Juno:

channel-1: 10 Osmo -> Juno
channel-2: 20 Osmo -> Juno
channel-3: 30 Osmo -> Juno

I would like to be find that its been 60 osmo IBC'd out, with no iteration.

closes: #2664
closes: #3187

Commit Message / Changelog Entry

Chore: Make a state entry to find total amount of native assets IBC'd out

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

damiannolan · 2023-01-18T08:00:38Z

Awesome! Thanks for the PR @stackman27, will try to review this soon!

nicolaslara

Added some comments about name changes for clarity.

Other than that and getting CI to pass, this looks good to me

modules/apps/transfer/client/cli/query.go

modules/apps/transfer/types/keys.go

crodriguezvega

Thanks for this contribution, @stackman27! I have a few of questions/remarks:

It would be great to have some tests for this!
The escrowed amount for a particular denom would be set on the first transfer of that denom. So until a transfer is done, the query would return 0, right? Would you consider adding a migration that calculates the amount in escrow and sets it beforehand?
When native tokens return back to the source chain, the total amount in escrow would not be updated until there's a transfer. Should you maybe update the total amount in escrow in the OnRecvPacket callback as well? Similar comment in case the transfer times out or the acknowledgement received is an error, then the tokens are refunded, so maybe it's good to update the value as well in refundTokens. In general, I would try to think in what places this value needs to be updated so it doesn't get out of sync with the actual amount in escrow.
Reading the original issue, I think the implementation is missing one requirement: Add a safety check if this value is ever attempted to be serialized to a negative value.
Following on @nicolaslara comment about naming, I think it would be nice to keep it consistent and use TotalEscrow instead of IBCOut everywhere.

Another thing that maybe deserves some thinking: in case there are other applications or middleware (e.g. rate limiting) relying on this value, we should be careful that the value cannot be tampered with to fool those applications. For example: one possible situation that comes to mind is that a malicious actor could transfer native tokens directly to one of the escrow accounts for the denom, then the value that you calculate based on the balance doesn't exactly reflect the true amount of native tokens that have been transferred out of the chain.

crodriguezvega · 2023-01-19T03:03:56Z

modules/apps/transfer/keeper/relay.go

+		existingAmount := k.bankKeeper.GetBalance(ctx, escrowAddress, token.Denom)
+
+		// store the token about to be IBC'd Out here
+		k.SetIBCOutDenomAmount(ctx, token.GetDenom(), existingAmount.Amount)


Maybe I'm missing something, but I think that this is setting the total amount escrowed for token.Denom on a particular channel, not across all channels, right? In the example you mention in the description of the PR, I think this code would set the total amount in escrow in channel-1, channel-2 or channel-3 depending on what channel the transfer is going out at any given time. For example, if I do a transfer on channel-1 it sets the total amount in escrow for channel-1, but if afterwards I do a transfer on channel-2 then the previous vale would get overwritten with the total amount in escrow for channel-2...

aah i see, i went with the assumption that all channels share same escrowAccount. So here, i'll probably have to get all the channel's escrow account balance sum it up and then store in every transfer right?

Yes, exactly. Tokens can be transferred out of a chain to the same destination chain through different channels, so you'll have add up the the balances of potentially multiple escrow accounts.

sounds great. I have addressed most of it as per this comment

crodriguezvega · 2023-01-19T03:31:17Z

proto/ibc/applications/transfer/v1/query.proto

+
+  // TotalNativeIBCOut returns the total native token that's IBC'd out.
+  rpc TotalNativeIBCOut(QueryTotalNativeIBCOutRequest) returns (QueryTotalNativeIBCOutResponse) {
+    option (google.api.http).get = "/ibc/apps/transfer/v1/total_native_ibc_out/{denom}";


Maybe for naming consistency and in case in the future we want to have other queries related to denoms?

Suggested change

option (google.api.http).get = "/ibc/apps/transfer/v1/total_native_ibc_out/{denom}";

option (google.api.http).get = "/ibc/apps/transfer/v1/denoms/{denom}/total_escrow";

And would this work fine if the denom contains slashes?

Wondering also if the port_id should be part of the URI...

Since the tracking is only happening for the transfer port, I think it makes sense to keep the port out of the uri or hard-code it to "transfer". Otherwise it seems like we could get this data for other channels, for which it just won't be available

stackman27 · 2023-01-21T08:35:46Z

Thanks you for your feedback @crodriguezvega. I have few questions/comments

It would be great to have some tests for this!

Finalizing some tests. Want to make sure I have approach ACK

The escrowed amount for a particular denom would be set on the first transfer of that denom. So until a transfer is done, the query would return 0, right? Would you consider adding a migration that calculates the amount in escrow and sets it beforehand?

Addressed! here

When native tokens return back to the source chain, the total amount in escrow would not be updated until there's a transfer. Should you maybe update the total amount in escrow in the OnRecvPacket callback as well? Similar comment in case the transfer times out or the acknowledgement received is an error, then the tokens are refunded, so maybe it's good to update the value as well in refundTokens. In general, I would try to think in what places this value needs to be updated so it doesn't get out of sync with the actual amount in escrow.

Addressed! transfer, onRecvPacket, refund I originally thought we were only tracking total channel's IBC out instead of tracking channel's total escrow amount so it got me a little confused

Reading the original issue, I think the implementation is missing one requirement: Add a safety check if this value is ever attempted to be serialized to a negative value.

Addressed! here

Following on @nicolaslara comment about naming, I think it would be nice to keep it consistent and use TotalEscrow instead of IBCOut everywhere.

Working on this, I still am unsure if native denoms contains slash("/") or not. I imagine it to be a standalone denomination like uosmo, uatom etc

Another thing that maybe deserves some thinking: in case there are other applications or middleware (e.g. rate limiting) relying on this value, we should be careful that the value cannot be tampered with to fool those applications. For example: one possible situation that comes to mind is that a malicious actor could transfer native tokens directly to one of the escrow accounts for the denom, then the value that you calculate based on the balance doesn't exactly reflect the true amount of native tokens that have been transferred out of the chain.

I didn't really think about this beforehand because i thought this security was already in place. If a random actor transfers $$ to channel-0 escrowAddress without proper transfer/refund, doesnot that violate current security gurantee as well

modules/apps/transfer/keeper/migrations.go

crodriguezvega · 2023-01-23T12:01:14Z

Thanks very much for addressing the comment so quickly, @stackman27! ❤️ I will give the PR another review in the next couple of days. To reply to your last comment:

I originally thought we were only tracking total channel's IBC out instead of tracking channel's total escrow amount so it got me a little confused

This is a good question. We both had different interpretations of the requirements :) I assumed that we wanted to track the total amount of native assets that are outside of the chain (and hence the total balance of the escrow accounts would give us), but your interpretation might as well be correct (track how many tokens have flowed out of the chain regardless of whether they came back). I think it would be good to hear @ValarDragon thoughts on what he originally wanted to track when he wrote the issue. I am also curious to hear @womensrights opinion on this.

Working on this, I still am unsure if native denoms contains slash("/") or not. I imagine it to be a standalone denomination like uosmo, uatom etc

Native denoms might contain slashes, so we should support this. We had an issue in the past that we didn't support denom with slashes correctly and it was not possible to transferred them.

If a random actor transfers $$ to channel-0 escrowAddress without proper transfer/refund, doesnot that violate current security gurantee as well

I think at the moment this doesn't pose any risk. The only thing is that the tokens will stay in the escrow account, probably locked forever (or until governance decides to take them out) since there will be no counterparty vouchers created for them in any other chain. So basically the user will waste their tokens without causing any risk to the application. But I think my point still stands: applications monitoring the total amount escrowed, might be fooled if this value is tampered with. I will discuss this with the rest of the team.

stackman27 · 2023-01-26T07:22:56Z

This is a good question. We both had different interpretations of the requirements :) I assumed that we wanted to track the total amount of native assets that are outside of the chain (and hence the total balance of the escrow accounts would give us), but your interpretation might as well be correct (track how many tokens have flowed out of the chain regardless of whether they came back). I think it would be good to hear @ValarDragon thoughts on what he originally wanted to track when he wrote the issue. I am also curious to hear @womensrights opinion on this.

Hi @crodriguezvega had a chat with @nicolaslara about this and we are tracking "total amount of native assets that are outside of the chain (and hence the total balance of the escrow accounts would give us)". The new commit i pushed highlights those changes. Please lmk what you think about it

crodriguezvega

Thanks for continue working on this, @stackman27! I have left a few comments with for some things that I think still need some more work. Let me know what you think!

modules/apps/transfer/client/cli/query.go

modules/apps/transfer/keeper/migrations.go

modules/apps/transfer/keeper/grpc_query.go

modules/apps/transfer/keeper/relay.go

modules/apps/transfer/keeper/keeper.go

stackman27 · 2023-02-08T00:48:58Z

@crodriguezvega @nicolaslara apologies for the delay, i was busy with some other work.

However, I've addressed all the comments and this PR is ready for re-review :))

crodriguezvega · 2023-02-16T07:59:36Z

@crodriguezvega @nicolaslara apologies for the delay, i was busy with some other work.

However, I've addressed all the comments and this PR is ready for re-review :))

Hi @stackman27, thanks for addressing the comments! I've started reviewing again and there's a few small changes that I would like to do, but if you don't mind, I think it would help to move this PR faster if I push the changes directly to your branch. Is that ok?

There's also a scenario that I think it's not covered yet. Imagine the following situation for tokens traveling from chain A (where the tokens are native) to chain C:

chains: chain A (channel-0) -> (channel-0) chain B (channel-1) -> (channel-1) chain C
denom:  stake                  transfer/channel-0/stake           transfer/channel-1/transfer/channel-0/stake

When the tokens are transferred from chain B to chain C, the SendChainIsSource check on sendTransfer will pass for tokens with denom transfer/channel-0/stake, because the source channel is channel-1 (since tokens are now being transferred to chain C). And because this check passes, then an escrow amount will be stored in state for denom transfer/channel-0/stake. But this would not be desired behavior, since we only want to track native denoms. I think similar behavior is happening also for the packet callbacks.

I am writing some tests to confirm this, and if my reasoning is not wrong, then I will implement the necessary changes in the code to handle this scenario and I will also push them to your branch. I hope this is also ok. I will comment on the PR, as soon as I push the changes, so that you can also review.

I think I will be able to push to your branch, in case I can, I will also comment here to ask you to give me permissions.

Thanks a lot for all this work so far!

modules/apps/transfer/keeper/msg_server.go

modules/apps/transfer/keeper/migrations.go

crodriguezvega

@stackman27 I made some changes (most notably adding checks to e2e test, genesis migration) so this PR is ready for another review now. There are a few items pending that I will work on in a follow up PR.

crodriguezvega · 2023-03-29T19:37:03Z

e2e/tests/transfer/base_test.go

+	t.Run("escrow amount for native denom is updated", func(t *testing.T) {
+		actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom)
+		s.Require().NoError(err)
+		s.Require().Equal(math.ZeroInt(), actualTotalEscrow)


This is zero because the tokens are back from chain B.

should this be added as an in code comment?

crodriguezvega · 2023-03-29T19:39:29Z

modules/apps/transfer/keeper/genesis_test.go


-	for i := 0; i < 5; i++ {


I refactored this to make it more table-test like.

Can we pull these changes out into a separate pr to main?

crodriguezvega · 2023-03-29T19:45:50Z

modules/apps/transfer/types/genesis.go

 	host "github.com/cosmos/ibc-go/v7/modules/core/24-host"
 )

 // NewGenesisState creates a new ibc-transfer GenesisState instance.
-func NewGenesisState(portID string, denomTraces Traces, params Params) *GenesisState {
+func NewGenesisState(portID string, denomTraces Traces, params Params, denomEscrows sdk.Coins) *GenesisState {


After discussing with @colin-axner, we can merge this API breaking change to main and in the backport revert it and add an extra constructor that takes the extra parameter.

.github/workflows/e2e-upgrade.yaml

colin-axner

Thanks for all the followup fixes! First pass, will continue reviewing next week

modules/apps/transfer/keeper/grpc_query.go

.github/workflows/e2e-upgrade.yaml

colin-axner · 2023-04-06T15:09:33Z

modules/apps/transfer/types/keys.go

+	KeyTotalEscrowPrefix = "totalEscrow"
+	KeyDenomsPrefix      = "denoms"


I think I'd prefer it to be one key, I don't find the pattern used for channel/port id path's to be equivalent. In referenced case, channelEnds is a key for the 04-channel keeper. Here the totalEscrow and denoms are referencing the same key, is there a scenario when we use totalEscrow as a prefix for a different key? Otherwise I'd prefer it to just be totalEscrow/{denom}

proto/ibc/applications/transfer/v1/query.proto

modules/apps/transfer/keeper/genesis.go

colin-axner · 2023-04-06T15:23:57Z

modules/apps/transfer/keeper/genesis_test.go


-	for i := 0; i < 5; i++ {


Can we pull these changes out into a separate pr to main?

proto/ibc/applications/transfer/v1/genesis.proto

modules/apps/transfer/keeper/relay.go

modules/apps/transfer/keeper/migrations.go

colin-axner

Almost done! Thanks for all the work! I only have a few files left to review (migrations). I left many suggestions/comments. Let me know if you have any questions

colin-axner · 2023-04-11T10:39:00Z

modules/apps/transfer/keeper/relay.go

+		// get the existing total amount in escrow
+		currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())
+		newTotalEscrow := currentTotalEscrow.Add(token.Amount)
+
+		// store the new total amount in escrow
+		k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)


Maybe we can update the comments to indicate why we are doing this as opposed to what the function means. Maybe something like:

Suggested change

// get the existing total amount in escrow

currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())

newTotalEscrow := currentTotalEscrow.Add(token.Amount)

// store the new total amount in escrow

k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)

// track the total amount in escrow keyed by denomination to allow for efficient iteration

currentTotalEscrow := k.GetTotalEscrowForDenom(ctx, token.GetDenom())

newTotalEscrow := currentTotalEscrow.Add(token.Amount)

k.SetTotalEscrowForDenom(ctx, token.GetDenom(), newTotalEscrow)

I wonder if we should dump the attached issue into an ADR and then reference the ADR number. If there's an architectural change a few years from now that makes this extra storing unnecessary, it might help to have context of why we made this decision

modules/apps/transfer/keeper/relay_test.go

colin-axner · 2023-04-11T10:42:34Z

modules/apps/transfer/keeper/relay_test.go

@@ -149,16 +157,95 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
 	}
 }

+func (suite *KeeperTestSuite) TestSendTransferSetsTotalEscrowAmountForSourceIBCToken() {


What is this test doing that TestSendTransfer isn't? I think this could be a test case of TestSendTransfer which uses a helper function to generate an IBC token that is sent to the third chain (or different channel in this case)?

Yes, I could look into that, but to be honest in my opinion the TestSendTransfer is a bit overloaded already (I was actually thinking of opening a PR with a suggestion to split it, since every time I have to come back to it I have a hard time understanding the tests cases)...

Simplifying the tests makes sense to me, I agree these tests are hard to follow. My concern is just having 1 test per test case. Did you open an issue?

modules/apps/transfer/keeper/relay_test.go

colin-axner · 2023-04-11T10:46:03Z

modules/apps/transfer/keeper/relay_test.go

+				amount = math.NewInt(50)
+				expEscrowAmount = math.NewInt(50)


What's the reasoning for this test case?

If I send back the full amount I wouldn't be sure of whether the subtraction happened correctly when the tokens are sent back or something else is wrong in the code, since there could be a situation where we have a bug in setting the escrow amount both on send and recv and the result would be the same as if it worked (the total amount in escrow at the end is 0).

Ah, this is true because recvIsSource. Should we add a comment?

modules/apps/transfer/keeper/keeper.go

colin-axner · 2023-04-11T11:13:12Z

e2e/tests/transfer/base_test.go

+	t.Run("escrow amount for native denom is updated", func(t *testing.T) {
+		actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom)
+		s.Require().NoError(err)
+		s.Require().Equal(math.ZeroInt(), actualTotalEscrow)


should this be added as an in code comment?

e2e/tests/transfer/base_test.go

modules/apps/transfer/keeper/grpc_query_test.go

damiannolan

Just flagging import grouping nits as I expect you would like to merge and complete the remainder with follow ups.

Awesome work on this! Thanks @stackman27 @crodriguezvega

modules/apps/transfer/keeper/genesis_test.go

modules/apps/transfer/keeper/migrations_test.go

crodriguezvega

Thanks a lot for all your work, @stackman27. We will continue with #3368 to wrap up the feature.

…out (#3019) (cherry picked from commit fa9418f) # Conflicts: # .github/workflows/e2e-upgrade.yaml # e2e/go.mod # e2e/tests/transfer/base_test.go # e2e/tests/upgrades/upgrade_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/types/genesis.pb.go # proto/ibc/applications/transfer/v1/genesis.proto # proto/ibc/applications/transfer/v1/transfer.proto

colin-axner · 2023-04-24T14:02:10Z

e2e/tests/transfer/base_test.go

+		// actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom)
+		// s.Require().NoError(err)


Is there an issue for these? Why are they commented out?

There is a proto deserialisation issue. See: this thread

colin-axner · 2023-04-24T14:05:45Z

modules/apps/transfer/keeper/genesis_test.go


+	for _, pathAndEscrowMount := range pathsAndEscrowAmounts {


typo? pathAndEscrowAmount?

colin-axner · 2023-04-24T14:07:16Z

modules/apps/transfer/keeper/genesis_test.go

 	}

 	genesis := suite.chainA.GetSimApp().TransferKeeper.ExportGenesis(suite.chainA.GetContext())

 	suite.Require().Equal(types.PortID, genesis.PortId)
 	suite.Require().Equal(traces.Sort(), genesis.DenomTraces)
+	suite.Require().Equal(escrows.Sort(), genesis.TotalEscrowed)

 	suite.Require().NotPanics(func() {
 		suite.chainA.GetSimApp().TransferKeeper.InitGenesis(suite.chainA.GetContext(), *genesis)


Is there a test for InitGenesis?

colin-axner · 2023-04-24T14:10:47Z

modules/apps/transfer/keeper/grpc_query_test.go

+			true,
+		},
+		{
+			"valid ibc denom treated as native denom",


what is this test case checking? The total escrow is empty for this denom

colin-axner · 2023-04-24T14:10:58Z

modules/apps/transfer/keeper/grpc_query_test.go

+			true, // denom trace is not found, thus the denom is considered a native token
+		},
+		{
+			"invalid ibc denom treated as valid native denom",


colin-axner · 2023-04-24T14:36:44Z

modules/apps/transfer/keeper/relay_test.go

@@ -149,16 +157,95 @@ func (suite *KeeperTestSuite) TestSendTransfer() {
 	}
 }

+func (suite *KeeperTestSuite) TestSendTransferSetsTotalEscrowAmountForSourceIBCToken() {


Simplifying the tests makes sense to me, I agree these tests are hard to follow. My concern is just having 1 test per test case. Did you open an issue?

colin-axner · 2023-04-24T14:44:47Z

modules/apps/transfer/keeper/relay_test.go

+			"tries to unescrow more tokens than allowed",
+			func() {
+				amount = sdk.NewInt(1000000)
+				expEscrowAmount = math.NewInt(100)


Oh this is only true because recvIsSource is set to true. Should we add a comment?

colin-axner · 2023-04-24T14:46:22Z

modules/apps/transfer/keeper/relay_test.go

+				amount = math.NewInt(50)
+				expEscrowAmount = math.NewInt(50)


Ah, this is true because recvIsSource. Should we add a comment?

colin-axner · 2023-04-24T14:48:17Z

modules/apps/transfer/keeper/relay_test.go

@@ -252,6 +383,17 @@ func (suite *KeeperTestSuite) TestOnRecvPacket() {

 			err = suite.chainB.GetSimApp().TransferKeeper.OnRecvPacket(suite.chainB.GetContext(), packet, data)

+			// check total amount in escrow of received token denom on receiving chain
+			var denom string
+			var totalEscrow math.Int


I don't think this needs separate declaration?

colin-axner · 2023-04-24T14:51:56Z

modules/apps/transfer/keeper/relay_test.go

+	escrowAddress := types.GetEscrowAddress(path2.EndpointB.ChannelConfig.PortID, path2.EndpointB.ChannelID)
+	coin := sdk.NewCoin(denomTrace.IBCDenom(), amount)
+	suite.Require().NoError(
+		banktestutil.FundAccount(


I find tests to be more maintainable when we use transfer logic to setup the state we want to test from. This code seems to be manually inserting ibc tokens rather then sending them between channels. If a feature is added, it'd need to modify this setup logic even if this test doesn't test the feature

… out (backport #3019) (#3505) * feat: make a state entry to find total amount of native assets IBC'd out (#3019) (cherry picked from commit fa9418f) # Conflicts: # .github/workflows/e2e-upgrade.yaml # e2e/go.mod # e2e/tests/transfer/base_test.go # e2e/tests/upgrades/upgrade_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/types/genesis.pb.go # proto/ibc/applications/transfer/v1/genesis.proto # proto/ibc/applications/transfer/v1/transfer.proto * fixing conflicts * linter fix * fix * remove api breaking change * address review comments * chore: remove unnecessary file --------- Co-authored-by: Sishir Giri <[email protected]> Co-authored-by: Carlos Rodriguez <[email protected]> Co-authored-by: Colin Axnér <[email protected]>

stackman27 force-pushed the stack/total-ibc-out branch from bdcb5b6 to 5d4f691 Compare January 17, 2023 21:53

stackman27 marked this pull request as ready for review January 17, 2023 21:53

stackman27 requested review from colin-axner, AdityaSripal and damiannolan as code owners January 17, 2023 21:53

damiannolan requested review from chatton, charleenfei and crodriguezvega January 18, 2023 08:01

nicolaslara reviewed Jan 18, 2023

View reviewed changes

modules/apps/transfer/client/cli/query.go Outdated Show resolved Hide resolved

modules/apps/transfer/types/keys.go Outdated Show resolved Hide resolved

stackman27 force-pushed the stack/total-ibc-out branch from 5d4f691 to 590593f Compare January 19, 2023 00:05

crodriguezvega reviewed Jan 19, 2023

View reviewed changes

stackman27 force-pushed the stack/total-ibc-out branch from 590593f to f2aa9e0 Compare January 21, 2023 08:31

stackman27 commented Jan 21, 2023

View reviewed changes

modules/apps/transfer/keeper/migrations.go Outdated Show resolved Hide resolved

crodriguezvega reviewed Jan 28, 2023

View reviewed changes

stackman27 force-pushed the stack/total-ibc-out branch 2 times, most recently from dc1a91c to 794801b Compare February 8, 2023 00:47

stackman27 requested review from nicolaslara and crodriguezvega and removed request for chatton, charleenfei and nicolaslara February 8, 2023 00:49

nicolaslara mentioned this pull request Feb 13, 2023

Modify calculated channel value once total escrowed amounts are added to IBC osmosis-labs/osmosis#3677

Open

crodriguezvega reviewed Feb 17, 2023

View reviewed changes

modules/apps/transfer/keeper/msg_server.go Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Feb 17, 2023

View reviewed changes

modules/apps/transfer/keeper/migrations.go Fixed Show fixed Hide fixed

crodriguezvega mentioned this pull request Mar 30, 2023

Follow up items for #3019 #3368

Closed

7 tasks

crodriguezvega reviewed Mar 30, 2023

View reviewed changes

colin-axner mentioned this pull request Apr 6, 2023

Code consistency nits in grpc handler #3422

Closed

3 tasks

colin-axner reviewed Apr 6, 2023

View reviewed changes

colin-axner reviewed Apr 11, 2023

View reviewed changes

Carlos Rodriguez added 8 commits April 11, 2023 23:01

addressing a bunch of review comments

1574d3f

addressing more review comments

540e3ac

review comment

7310277

Merge branch 'main' into stack/total-ibc-out

07d3266

e2e: add transfer query client

b3fcf69

change grpc gateway URI for total escrow

8057568

position nit

e3267e8

address walkthrough comments

a0757c9

damiannolan approved these changes Apr 18, 2023

View reviewed changes

modules/apps/transfer/keeper/genesis_test.go Outdated Show resolved Hide resolved

modules/apps/transfer/keeper/migrations_test.go Outdated Show resolved Hide resolved

import formatting

5ba4f59

crodriguezvega requested a review from DimitrisJim as a code owner April 18, 2023 21:38

Carlos Rodriguez added 4 commits April 22, 2023 10:02

comment out failing e2e test for now / rename function / add comments

4047be9

Merge branch 'main' into stack/total-ibc-out

77655f2

rename receiver

9c036e4

rename receiver (2)

aac5321

crodriguezvega added the backport-to-v7.1.x label Apr 22, 2023

comment out import

107f4d6

crodriguezvega approved these changes Apr 22, 2023

View reviewed changes

crodriguezvega merged commit fa9418f into cosmos:main Apr 22, 2023

mergify bot mentioned this pull request Apr 22, 2023

Chore: Make a state entry to find total amount of native assets IBC'd out (backport #3019) #3505

Merged

colin-axner reviewed Apr 24, 2023

View reviewed changes

colin-axner mentioned this pull request May 3, 2023

Make explicit decision on removing or keeping 0 int's in state for total escrow amount #3549

Closed

3 tasks

faddat mentioned this pull request Jun 23, 2023

v7.2.0 for sdk 50 #3948

Closed

9 tasks

	option (google.api.http).get = "/ibc/apps/transfer/v1/total_native_ibc_out/{denom}";
	option (google.api.http).get = "/ibc/apps/transfer/v1/denoms/{denom}/total_escrow";

		KeyTotalEscrowPrefix = "totalEscrow"
		KeyDenomsPrefix = "denoms"

		// actualTotalEscrow, err := s.QueryTotalEscrowForDenom(ctx, chainA, chainADenom)
		// s.Require().NoError(err)

Chore: Make a state entry to find total amount of native assets IBC'd out #3019

Chore: Make a state entry to find total amount of native assets IBC'd out #3019

Conversation

stackman27 commented Jan 16, 2023 • edited by crodriguezvega Loading

Description

Commit Message / Changelog Entry

damiannolan commented Jan 18, 2023

nicolaslara left a comment

Choose a reason for hiding this comment

crodriguezvega left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolaslara Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

stackman27 commented Jan 21, 2023 • edited Loading

crodriguezvega commented Jan 23, 2023

stackman27 commented Jan 26, 2023 • edited Loading

crodriguezvega left a comment

Choose a reason for hiding this comment

stackman27 commented Feb 8, 2023

crodriguezvega commented Feb 16, 2023 • edited Loading

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stackman27 commented Jan 16, 2023 •

edited by crodriguezvega

Loading

crodriguezvega left a comment •

edited

Loading

stackman27 Jan 19, 2023 •

edited

Loading

nicolaslara Jan 19, 2023 •

edited

Loading

stackman27 commented Jan 21, 2023 •

edited

Loading

stackman27 commented Jan 26, 2023 •

edited

Loading

crodriguezvega commented Feb 16, 2023 •

edited

Loading

crodriguezvega Apr 15, 2023 •

edited

Loading