feat: add metadata for IBC tokens #3104

0xmuralik · 2023-02-03T09:38:07Z

Description

Commit Message / Changelog Entry

type: commit message

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 and Go style guide.
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.

codecov-commenter · 2023-02-03T10:01:46Z

Codecov Report

❗ No coverage uploaded for pull request base (main@b53bfa5). Click here to learn what that means.
The diff coverage is 94.44%.

❗ Current head ec3d294 differs from pull request most recent head aaa6786. Consider uploading reports for the commit aaa6786 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3104   +/-   ##
=======================================
  Coverage        ?   79.31%           
=======================================
  Files           ?      188           
  Lines           ?    13081           
  Branches        ?        0           
=======================================
  Hits            ?    10375           
  Misses          ?     2271           
  Partials        ?      435

Files Changed	Coverage Δ
modules/apps/transfer/module.go	`46.55% <33.33%> (ø)`
modules/apps/transfer/keeper/genesis.go	`92.59% <100.00%> (ø)`
modules/apps/transfer/keeper/keeper.go	`91.83% <100.00%> (ø)`
modules/apps/transfer/keeper/migrations.go	`96.87% <100.00%> (ø)`
modules/apps/transfer/keeper/relay.go	`91.00% <100.00%> (ø)`

crodriguezvega

Thanks a lot for this, @0xmuralik! This will be a great UX improvement!

I am wondering if we should have also a migration to add the metadata for the existing denom traces. We can fetch them using GetAllDenomTraces. That way we would have metadata available already without the need for some tokens to be transferred. What do you think?

modules/apps/transfer/keeper/relay.go

0xmuralik · 2023-02-06T10:53:39Z

Thanks a lot for this, @0xmuralik! This will be a great UX improvement!

I am wondering if we should have also a migration to add the metadata for the existing denom traces. We can fetch them using GetAllDenomTraces. That way we would have metadata available already without the need for some tokens to be transferred. What do you think?

Sounds good. We can set MetaData in the initgenesis here after SetDenomTrace.

crodriguezvega · 2023-02-07T09:22:22Z

Sounds good. We can set MetaData in the initgenesis here after SetDenomTrace.

Yes, true, we should also do it in InitGenesis!

But wouldn't we also need a migration handler for chain upgrades? We could run this as an automatic migration handler by bumping the consensus version of transfer.

colin-axner · 2023-02-15T16:23:57Z

modules/apps/transfer/keeper/migrations.go

+
+			if !m.keeper.bankKeeper.HasDenomMetaData(ctx, newTrace.IBCDenom()) {
+				m.keeper.SetDenomMetaData(ctx, newTrace)
+			}
+


This migration may have already been run and might not be run again. I think it is safer to make a separate migration only setting the denom metadata. Please let me know if you have any questions on how to do this. I'm also happy to receive that change in a followup pr, it would make reviews faster/easier

I have created a new migration and registered it with as version 3 migration. Feel free to take up the task if required.

colin-axner · 2023-02-15T16:26:04Z

modules/apps/transfer/keeper/relay.go

+		Description: getMetaDataDescription(denomTrace),
+		DenomUnits: []*banktypes.DenomUnit{
+			{
+				Denom:    denomTrace.BaseDenom,
+				Exponent: 0,
+			},
+		},
+		// Setting base as IBChash Denom as SetDenomMetaData uses Base as storeKey
+		// and the bank keeper will only have the IBCHash to get the denom metadata
+		Base:    denomTrace.IBCDenom(),
+		Display: denomTrace.GetFullDenomPath(),
+		Name:    getMetaDataName(denomTrace),
+		Symbol:  getMetaDataSymbol(denomTrace),


Can we have an example print out of how this might look? Would be great if we could get product input on the desired formatting? cc @womensrights @adiraviraj @tmsdkeys

For context, this pr would add metadata about IBC tokens into the state machine. This means when clients query a bank balance, instead of only seeing ibc/{hash} they could also provide a flag to obtain back the metadata information stored here, such as the base denomination or the full history of this tokens hops (via channel/portIDs)

I added a unit test for the new migration. The example metadata is available here.

I've known people asking for this on Discord, to have the metadata about IBC tokens as you would regular sdk tokens so think it's a good idea. Wrt the formatting, looks okay for me. The Base field should normally equal DenomUnits.Denom with Exponent according to the bank module's metadata, but the reasoning is provided why it needs to deviate.
Rest looks okay for me. Maybe @womensrights has insights from user research?

I think this is a great first step, but also adding in a human readable name to the current output would precisely cover what the metadata is used for. Front end developers are currently maintaining their own registries of assets to be displayed for their application. An example of such a registry is for Injective. Front ends need:

token symbol, this is covered

the location of the decimal place for the denom, I think this is covered from the exponent, from my understanding, e.g. the precision of ATOM is 6 dp, ATOM exponent is 6, uATOM is 0. In the example output given its not clear though as you have used an exponent of 0 for ubar and foo, foo should have exponent 6 assuming it is an sdk coin?

human readable name, having the path is good, but then you would need to check which chain the token came from additionally to make this human readable

So I think this is already an improvement, just if there was also a way to add an additional human readable name somehow then this would be great but I'm not sure this is easily done. Also as a sidenote, I know there is a constraint on the metadata from the sdk types so this is not easy to change, but for the ibc denoms, you don't get any new information from looking at the Display compared to Name so in the long term I would question the need to have both

Thank you for the input, @womensrights!

Regarding the exponent, if I am not mistaken I think at the moment it will be difficult to set it to the correct value, since this metadata is set on the receiving chain and the FungibleTokenPacketData does not contain that information at the moment. However, with channel upgradability it would be possible to extend the packet data and add extra information.

Regarding the lack of chain ID. Maybe the sending chain's chain ID could be added in the memo field? Not that I particularly want to continue adding more information in the memo file :), but maybe it's a solution for now. Your comment if there was also a way to add an additional human readable name somehow refers to adding the chain ID, right?

For the exponent, as you linked the FungibleTokenPacketData there isn't explicitly an exponent field but where does the logic to know the decimal place for the amount in the packet for a specific denom come from in this case?

The chainID would also not completely work as sometimes the chainID is not the same as what is considered the human branded name for the chain or you would need to remove the additional numbers. e.g. I think bandchain has chainID "laozi-mainnet" and the human readable name would be Bandchain

For the exponent, as you linked the FungibleTokenPacketData there isn't explicitly an exponent field but where does the logic to know the decimal place for the amount in the packet for a specific denom come from in this case?

The amount comes from the sdk Coin that is used in MsgTransfer. Whatever that amount is, we put it in the FungibleTokenPacketData, but we don't have any information about the exponent.

The chainID would also not completely work as sometimes the chainID is not the same as what is considered the human branded name for the chain or you would need to remove the additional numbers. e.g. I think bandchain has chainID "laozi-mainnet" and the human readable name would be Bandchain

True, good point! Then I guess we cannot do anything about this at the moment?

ok I see, I guess this adr has not yet been implemented and right now its assumed that sdk coins all have 6 decimal places.

Yes I don't think there can be anything done about the chainIDs

colin-axner

Thanks @0xmuralik! :)

0xmuralik · 2023-02-23T10:04:32Z

@colin-axner @crodriguezvega If there are any additional changes required please let me know. Else fell free to take up this task in a different PR and close this one if necesssary.

colin-axner

LGTM but see comments. More test assertions are needed and I'd recommend blending the added functionality into the existing SetDenomTrace function to avoid accidental bugs in the future due to not calling both SetDenomTrace and SetMetadata

modules/apps/transfer/keeper/migrations_test.go

colin-axner · 2023-08-31T08:29:52Z

modules/apps/transfer/keeper/relay.go

@@ -277,6 +278,10 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, data t
 		return errorsmod.Wrap(err, "failed to mint IBC tokens")
 	}

+	if !k.bankKeeper.HasDenomMetaData(ctx, voucherDenom) {


if we are running migrations such that all traces have metadata set, shouldn't we just make setting metadata a functionality of SetDenomTrace? This avoids accidental incidents where the trace is set but the metadata isn't. There should never be an instance when you want to set the denom trace without setting the metadata as well?

I guess we could do that, although SetDenomTrace is a setter function for a specific key in the store, so it would squeak to me a bit if we add SetDenomMetadata there as well. Maybe we could have a helper function that wraps both SetDenomTrace and SetDenomMetadata. Happy to hear other people's thoughts.

modules/apps/transfer/keeper/relay.go

modules/apps/transfer/keeper/migrations.go

modules/apps/transfer/keeper/genesis.go

modules/apps/transfer/keeper/relay.go

crodriguezvega · 2023-09-04T12:09:18Z

modules/apps/transfer/keeper/relay_test.go

-			var (
-				denom       string
-				totalEscrow sdk.Coin
-			)
-			if tc.recvIsSource {
-				denom = sdk.DefaultBondDenom
-			} else {
-				denom = trace.IBCDenom()
-			}
-			totalEscrow = suite.chainB.GetSimApp().TransferKeeper.GetTotalEscrowForDenom(suite.chainB.GetContext(), denom)


I realised that this could be simplified. It was actually testing the wrong thing, I think, but looks like by chance the tests passed...

modules/apps/transfer/keeper/relay_test.go

modules/apps/transfer/keeper/relay.go

…pp rendering the assertion unnecessary

colin-axner

LGTM, thank you @0xmuralik and @crodriguezvega!

modules/apps/transfer/keeper/relay_test.go

damiannolan

LGTM! Looks good to me, left some suggestions which were only nits

modules/apps/transfer/keeper/genesis_test.go

modules/apps/transfer/keeper/keeper.go

modules/apps/transfer/module.go

chatton

LGTM thanks @0xmuralik, just left a few small comments but it looks great 👍

modules/apps/transfer/keeper/migrations.go

modules/apps/transfer/keeper/keeper.go

crodriguezvega · 2023-09-11T20:54:49Z

Thanks for the reviews, @colin-axner, @damiannolan and @chatton. I addressed all the comments and the PR is then ready for merge after CI passes.

0xmuralik added 3 commits February 2, 2023 15:14

add metadata after mint

b1359b2

set base denom as ibc hash denom

4070c24

comment

d0c2b83

0xmuralik requested review from colin-axner, AdityaSripal and damiannolan as code owners February 3, 2023 09:38

0xmuralik mentioned this pull request Feb 3, 2023

feat: resolve IBC hash to human readable denom cosmos/cosmos-sdk#14894

Merged

19 tasks

0xmuralik and others added 2 commits February 3, 2023 16:32

remove unsed funcs from keeper

d0662a4

Merge branch 'main' into murali/ibc-denom

ef8310a

crodriguezvega reviewed Feb 6, 2023

View reviewed changes

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

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

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

genesis setmetadata

e460e35

crodriguezvega added this to the v8.0.0 milestone Feb 6, 2023

0xmuralik and others added 2 commits February 10, 2023 12:06

set metadata in migration

850ded9

Merge branch 'main' into murali/ibc-denom

9e7924a

0xmuralik requested a review from crodriguezvega February 14, 2023 04:56

Merge branch 'main' into murali/ibc-denom

e257dcb

colin-axner reviewed Feb 15, 2023

View reviewed changes

colin-axner changed the title ~~add metadata after mint for IBC token~~ add metadata for IBC tokens Feb 15, 2023

colin-axner changed the title ~~add metadata for IBC tokens~~ feat: add metadata for IBC tokens Feb 15, 2023

0xmuralik added 2 commits February 16, 2023 10:41

new migration

21c81e5

test new migration

dafd5f1

0xmuralik requested review from colin-axner and removed request for crodriguezvega February 16, 2023 06:10

Merge branch 'main' into murali/ibc-denom

7763f74

crodriguezvega added the v8 label Aug 30, 2023

colin-axner reviewed Aug 31, 2023

View reviewed changes

Carlos Rodriguez added 3 commits August 31, 2023 17:07

addressing some of the feedback

771dd3c

add more test cases

e94f138

rename variables

2794a64

crodriguezvega assigned crodriguezvega and unassigned DimitrisJim and crodriguezvega Sep 4, 2023

check metadata in onrecvpacket tests

b97b8ee

crodriguezvega reviewed Sep 4, 2023

View reviewed changes

modules/apps/transfer/keeper/relay_test.go Show resolved Hide resolved

crodriguezvega reviewed Sep 4, 2023

View reviewed changes

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

colin-axner reviewed Sep 6, 2023

View reviewed changes

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

colin-axner and others added 5 commits September 6, 2023 13:25

fix: remove test assertion on errors, state will be reverted by basea…

bcfe839

…pp rendering the assertion unnecessary

Merge branch 'main' into murali/ibc-denom

b8e0f63

address review comment

7091998

lint, lint, lint

ab7c94f

Merge branch 'main' into murali/ibc-denom

a433aad

colin-axner approved these changes Sep 7, 2023

View reviewed changes

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

colin-axner and others added 3 commits September 7, 2023 15:47

Merge branch 'main' into murali/ibc-denom

ca1304b

fix test

1c2626d

Merge branch 'main' into murali/ibc-denom

0e1c34e

damiannolan approved these changes Sep 11, 2023

View reviewed changes

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

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

modules/apps/transfer/module.go Show resolved Hide resolved

chatton approved these changes Sep 11, 2023

View reviewed changes

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

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

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

Carlos Rodriguez added 2 commits September 11, 2023 22:42

address review comments

b22484a

Merge branch 'main' into murali/ibc-denom

ec3d294

Merge branch 'main' into murali/ibc-denom

aaa6786

crodriguezvega merged commit 99c16eb into cosmos:main Sep 12, 2023
51 checks passed

chatton mentioned this pull request Sep 13, 2023

V7 to V8 Upgrade Test Part #2 #4645

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add metadata for IBC tokens #3104

feat: add metadata for IBC tokens #3104

0xmuralik commented Feb 3, 2023

codecov-commenter commented Feb 3, 2023 •

edited

Loading

crodriguezvega left a comment

0xmuralik commented Feb 6, 2023

crodriguezvega commented Feb 7, 2023

colin-axner Feb 15, 2023

0xmuralik Feb 16, 2023

colin-axner Feb 15, 2023

colin-axner Feb 15, 2023

0xmuralik Feb 16, 2023

tmsdkeys Feb 23, 2023

womensrights Jun 13, 2023

crodriguezvega Jun 16, 2023

womensrights Jun 19, 2023

crodriguezvega Jun 20, 2023

womensrights Jun 22, 2023

colin-axner left a comment

0xmuralik commented Feb 23, 2023

colin-axner left a comment

colin-axner Aug 31, 2023

crodriguezvega Sep 4, 2023

crodriguezvega Sep 4, 2023

colin-axner left a comment

damiannolan left a comment

chatton left a comment

crodriguezvega commented Sep 11, 2023 •

edited

Loading

feat: add metadata for IBC tokens #3104

feat: add metadata for IBC tokens #3104

Conversation

0xmuralik commented Feb 3, 2023

Description

Commit Message / Changelog Entry

codecov-commenter commented Feb 3, 2023 • edited Loading

Codecov Report

crodriguezvega left a comment

Choose a reason for hiding this comment

0xmuralik commented Feb 6, 2023

crodriguezvega commented Feb 7, 2023

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

colin-axner left a comment

Choose a reason for hiding this comment

0xmuralik commented Feb 23, 2023

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

colin-axner left a comment

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

crodriguezvega commented Sep 11, 2023 • edited Loading

codecov-commenter commented Feb 3, 2023 •

edited

Loading

crodriguezvega commented Sep 11, 2023 •

edited

Loading