feat: use namada registry + IBC decoding fixes #1240

emccorson · 2024-11-11T09:55:27Z

This PR integrates Namadillo with Namada registry, as well as fixing some other things related to decoding addresses to assets. This adds support for holding the same asset from multiple chains. To test, you can transfer some NAM from the internal devnet to cosmoshub testnet to housefire, and you should see both your housefire NAM and your internal devnet NAM listed on your account.

The package.json points to a specific branch of namada-chain-registry, but I can update this as soon as anoma/namada-chain-registry#3 is merged. The code integrating the registry is a bit clunky, but this can be improved as the registry develops.

Added

Use Namada chain registry. Closes Namadillo: Integrate Namada chain registry #1217

Fixed

Index assets by their original address, not by base denom. Fixes Namadillo: IBC -> Allow decoding the same asset from multiple chains #1220
- Also refactor TransferModule accordingly
Look up registry asset by base denom, alias or address
Use address to index assets in 'Select Asset' list

apps/namadillo/package.json

euharrison · 2024-11-11T19:23:22Z

apps/namadillo/src/types.d.ts

@@ -189,13 +189,16 @@ export type ChainRegistryEntry = {
  ibc: IBCInfo[];
 };

-export type AddressWithAssetAndBalance = {
-  address: string;
+export type AddressWithAsset = {


To simplify some names, I think we could call this as Token, wdyt?

My reference is the tokens.toml files that is used by the cli, that is basically the tnam... address plus some properties

Not sure if Token should be the best name for it, but I agree we should simplify this name. What about AssetRegistry ?

I definitely agree the names I used are not very good. I'm not sure about Token since I think we have other things named similar to Token (but maybe those are not needed now, I have to check). AssetRegistry makes me think more of the full registry, like cosmos/chain-registry.

For now, I created #1244 to track this and I think it should be fairly easy to change the names when we settle on something.

Some ChatGPT recommendations:

AssetRecord (address and asset only) and AssetAmountRecord (address, asset, and amount)

AssetReference (address and asset only) and AssetAmountEntry (address, asset, and amount)

ResolvedAsset (address and asset only) and ResolvedAssetAmount (address, asset, and amount)

TokenEntry (address and asset only) and TokenAmountEntry (address, asset, and amount)

I'm not sure about these since they don't indicate that there is an original address, but maybe they're OK.

pedrorezende

Amazing PR! Just a few comments on variables / property names! Great work 🚀

pedrorezende · 2024-11-12T05:35:47Z

apps/namadillo/src/types.d.ts

-export type AddressWithAssetAndBalanceMap = Record<
-  string,
-  AddressWithAssetAndBalance
+export type AddressWithAssetAndAmount = AddressWithAsset & {


Still on names, maybe this one could be something like FungibleAsset (?) . We will need to differentiate this in the future for nfts

pedrorezende · 2024-11-12T05:38:59Z

apps/namadillo/src/types.d.ts

@@ -189,13 +189,16 @@ export type ChainRegistryEntry = {
  ibc: IBCInfo[];
 };

-export type AddressWithAssetAndBalance = {
-  address: string;
+export type AddressWithAsset = {


Not sure if Token should be the best name for it, but I agree we should simplify this name. What about AssetRegistry ?

apps/namadillo/src/registry/namadaAsset.ts

pedrorezende · 2024-11-12T05:43:19Z

apps/namadillo/src/integrations/utils.ts

@@ -44,7 +44,8 @@ export const getTransactionFee = (
    if (typeof asset !== "undefined") {
      return {
        amount: BigNumber(0.003), // TODO: remove hardcoding
-        token: asset,
+        asset,
+        address: asset.base,


Sorry, reading the code, this property name seems to be more suitable to something like denom or base, once it's not handling the asset address. We might get confused in the future

I have added this to #1244 for tracking. I think one problem is that denom, base and address all exist on the Asset type, but what we are calling the address might actually be none of those things. The intention of the property is to be "the thing we pass to Namada/Cosmos SDK to tell it what to transfer", but I have no idea what to call that 😄

For now I'll make this originalAddress so it's hopefully less confusing, but this can just be a placeholder until we get something better.

apps/namadillo/src/App/Transfer/__mocks__/assets.tsx

- Use Namada chain registry. Closes #1217 - Index assets by their original address, not by base denom. Fixes #1220 - Look up registry asset by base denom, alias or address - Use address to index assets in 'Select Asset' list

emccorson force-pushed the integrate-namada-registry branch from 357ccf1 to 338041f Compare November 11, 2024 10:16

emccorson changed the title ~~feat: integrate with namada registry~~ feat: use namada registry + IBC decoding fixes Nov 11, 2024

emccorson marked this pull request as ready for review November 11, 2024 10:47

emccorson requested review from pedrorezende, euharrison, jurevans and mateuszjasiuk and removed request for pedrorezende November 11, 2024 10:47

emccorson force-pushed the integrate-namada-registry branch from 338041f to a3adf75 Compare November 11, 2024 14:06

euharrison approved these changes Nov 11, 2024

View reviewed changes

pedrorezende approved these changes Nov 12, 2024

View reviewed changes

emccorson mentioned this pull request Nov 12, 2024

Namadillo: IBC -> Review type names #1244

Open

feat: use namada registry + IBC decoding fixes (#1240)

799e84c

- Use Namada chain registry. Closes #1217 - Index assets by their original address, not by base denom. Fixes #1220 - Look up registry asset by base denom, alias or address - Use address to index assets in 'Select Asset' list

emccorson force-pushed the integrate-namada-registry branch from fb265df to 799e84c Compare November 12, 2024 09:41

emccorson merged commit 799e84c into main Nov 12, 2024
9 checks passed

emccorson deleted the integrate-namada-registry branch November 12, 2024 09:41

github-actions bot temporarily deployed to production November 12, 2024 09:52 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use namada registry + IBC decoding fixes #1240

feat: use namada registry + IBC decoding fixes #1240

emccorson commented Nov 11, 2024 •

edited

Loading

euharrison Nov 11, 2024

pedrorezende Nov 12, 2024

emccorson Nov 12, 2024

emccorson Nov 12, 2024

pedrorezende left a comment

pedrorezende Nov 12, 2024

pedrorezende Nov 12, 2024

pedrorezende Nov 12, 2024

emccorson Nov 12, 2024

emccorson Nov 12, 2024

feat: use namada registry + IBC decoding fixes #1240

feat: use namada registry + IBC decoding fixes #1240

Conversation

emccorson commented Nov 11, 2024 • edited Loading

Added

Fixed

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

pedrorezende 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

emccorson commented Nov 11, 2024 •

edited

Loading