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

feat: handle nam as denominated amount #1301

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/namadillo/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@
/test-results/
/playwright-report/
/playwright/.cache/
/public/localnet-config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ const TransparentTokensTable = ({
>
Shield
</ActionButton>
{originalAddress === namadaAsset.address && (
{originalAddress === namadaAsset().address && (
<ActionButton
size="xs"
className="w-fit mx-auto"
Expand Down Expand Up @@ -127,7 +127,7 @@ const TransparentTokensTable = ({
};

const PanelContent = ({ data }: { data: TokenBalance[] }): JSX.Element => {
const namBalance = data.find((i) => i.asset.base === namadaAsset.base);
const namBalance = data.find((i) => i.asset.base === namadaAsset().base);

return (
<div className="flex flex-col gap-2">
Expand Down
4 changes: 2 additions & 2 deletions apps/namadillo/src/App/Ibc/IbcWithdraw.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ export const IbcWithdraw: React.FC = () => {

const transactionFee = mapUndefined(
({ gasLimit, gasPrice }) => ({
originalAddress: namadaAsset.address,
asset: namadaAsset,
originalAddress: namadaAsset().address,
asset: namadaAsset(),
amount: gasPrice.multipliedBy(gasLimit),
}),
gasConfig
Expand Down
3 changes: 3 additions & 0 deletions apps/namadillo/src/App/Setup/ChainLoader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { AtomErrorBoundary } from "App/Common/AtomErrorBoundary";
import { ErrorBox } from "App/Common/ErrorBox";
import { routes } from "App/routes";
import { chainAtom } from "atoms/chain";
import { localnetConfigAtom } from "atoms/integrations";
import { useAtomValue } from "jotai";
import { ReactNode } from "react";
import { useLocation, useNavigate } from "react-router-dom";
Expand Down Expand Up @@ -31,6 +32,8 @@ export const ChainLoader = ({
children: ReactNode;
}): JSX.Element => {
const chain = useAtomValue(chainAtom);
useAtomValue(localnetConfigAtom);

const errorContainerProps = {
className: "bg-black max-w-full rounded-sm w-full text-white min-h-full",
};
Expand Down
2 changes: 1 addition & 1 deletion apps/namadillo/src/App/Transfer/AssetImage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export const AssetImage = ({
<img src={image} alt={altText} className="w-full" />
{isShielded !== undefined && (
<span className="absolute -bottom-1 -right-1 w-4 aspect-square">
<AssetImage asset={namadaAsset} />
<AssetImage asset={namadaAsset()} />
</span>
)}
</span>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@ describe("Component: TransferDestination", () => {
<TransferDestination
transactionFee={{
amount: fee,
asset: namadaAsset,
originalAddress: namadaAsset.address,
asset: namadaAsset(),
originalAddress: namadaAsset().address,
}}
/>
);
Expand Down
7 changes: 1 addition & 6 deletions apps/namadillo/src/atoms/accounts/atoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { namadaExtensionConnectedAtom } from "atoms/settings";
import { queryDependentFn } from "atoms/utils";
import BigNumber from "bignumber.js";
import { atomWithMutation, atomWithQuery } from "jotai-tanstack-query";
import { chainConfigByName } from "registry";
import {
fetchAccountBalance,
fetchAccounts,
Expand Down Expand Up @@ -85,7 +84,6 @@ export const accountBalanceAtom = atomWithQuery<BigNumber>((get) => {
const tokenAddress = get(nativeTokenAddressAtom);
const enablePolling = get(shouldUpdateBalanceAtom);
const api = get(indexerApiAtom);
const chainConfig = chainConfigByName("namada");

return {
// TODO: subscribe to indexer events when it's done
Expand All @@ -95,10 +93,7 @@ export const accountBalanceAtom = atomWithQuery<BigNumber>((get) => {
return await fetchNamAccountBalance(
api,
defaultAccount.data,
tokenAddress.data!,
// As this is a nam balance specific atom, we can safely assume that the
// first currency is the native token
chainConfig.currencies[0].coinDecimals
tokenAddress.data!
);
}, [tokenAddress, defaultAccount]),
};
Expand Down
8 changes: 2 additions & 6 deletions apps/namadillo/src/atoms/accounts/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@ export const fetchDefaultAccount = async (): Promise<Account | undefined> => {
export const fetchNamAccountBalance = async (
api: DefaultApi,
account: Account | undefined,
tokenAddress: string,
decimals: number
tokenAddress: string
): Promise<BigNumber> => {
if (!account) return BigNumber(0);
const balancesResponse = await api.apiV1AccountAddressGet(account.address);

const balance = balancesResponse.data
// TODO: add filter to the api call
.filter(({ tokenAddress: ta }) => ta === tokenAddress)
.map(({ tokenAddress, balance }) => {
return {
Expand All @@ -34,9 +32,7 @@ export const fetchNamAccountBalance = async (
})
.at(0);

return balance ?
BigNumber(balance.amount).shiftedBy(-decimals)
: BigNumber(0);
return balance ? BigNumber(balance.amount) : BigNumber(0);
};

export const fetchAccountBalance = async (
Expand Down
6 changes: 1 addition & 5 deletions apps/namadillo/src/atoms/balance/atoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,7 @@ export const shieldedBalanceAtom = atomWithQuery<
);
const shieldedBalance = response.map(([address, amount]) => ({
address,
amount:
// Sdk returns the nam amount as `nam` instead of `namnam`
namTokenAddressQuery.data === address ?
new BigNumber(amount).shiftedBy(6)
: new BigNumber(amount),
amount: new BigNumber(amount),
}));
return shieldedBalance;
}, [viewingKeyQuery, tokenAddressesQuery, namTokenAddressQuery]),
Expand Down
2 changes: 1 addition & 1 deletion apps/namadillo/src/atoms/balance/functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export const getTotalDollar = (list?: TokenBalance[]): BigNumber | undefined =>
sumDollars(list ?? []);

export const getTotalNam = (list?: TokenBalance[]): BigNumber =>
list?.find((i) => i.asset.base === namadaAsset.base)?.amount ??
list?.find((i) => i.asset.base === namadaAsset().base)?.amount ??
new BigNumber(0);

const tnamAddressToDenomTrace = (
Expand Down
26 changes: 26 additions & 0 deletions apps/namadillo/src/atoms/integrations/atoms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
TransferTransactionData,
} from "types";
import {
addLocalnetToRegistry,
createIbcTx,
getIbcChannels,
getKnownChains,
Expand All @@ -28,6 +29,7 @@ import {
mapCoinsToAssets,
} from "./functions";
import {
fetchLocalnetTomlConfig,
IbcTransferParams,
queryAndStoreRpc,
queryAssetBalances,
Expand Down Expand Up @@ -198,3 +200,27 @@ export const createIbcTxAtom = atomWithMutation((get) => {
},
};
});

export const localnetConfigAtom = atomWithQuery((_get) => {
return {
queryKey: ["localnet-config"],
staleTime: Infinity,
retry: false,

queryFn: async () => {
try {
const { enabled, chain_id, token_address } =
await fetchLocalnetTomlConfig();

if (enabled && chain_id && token_address) {
addLocalnetToRegistry(chain_id, token_address);
}

return { chainId: chain_id, tokenAddress: token_address };
} catch (_) {
// If file not found just ignore
return null;
}
},
};
});
36 changes: 35 additions & 1 deletion apps/namadillo/src/atoms/integrations/functions.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Asset, Chain } from "@chain-registry/types";
import { Asset, AssetList, Chain } from "@chain-registry/types";
import { Coin } from "@cosmjs/launchpad";
import { QueryClient, setupIbcExtension } from "@cosmjs/stargate";
import { Tendermint34Client } from "@cosmjs/tendermint-rpc";
Expand Down Expand Up @@ -370,3 +370,37 @@ export const createIbcTx = async (
);
return transactionPair;
};

export const namadaLocal = (chainId: string): Chain => ({
...internalDevnetChain,
chain_name: "localnet",
chain_id: chainId,
});

export const namadaLocalAsset = (tokenAddress: string): AssetList => ({
...internalDevnetAssets,
chain_name: "localnet",
assets: internalDevnetAssets.assets.map((asset) =>
asset.name === "NAM" ?
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is not going to work once we're using the registry with anoma/namada-chain-registry#13 merged. Maybe could use "Namada" or check against symbol or base instead?

...asset,
address: tokenAddress,
}
: asset
),
});

export const addLocalnetToRegistry = (
chainId: string,
tokenAddress: string
): void => {
const localChain: ChainRegistryEntry = {
chain: namadaLocal(chainId),
assets: namadaLocalAsset(tokenAddress),
};

cosmosRegistry.chains.push(localChain.chain);
cosmosRegistry.assets.push(localChain.assets);

mainnetChains.push(localChain);
};
7 changes: 7 additions & 0 deletions apps/namadillo/src/atoms/integrations/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ import { queryForAck, queryForIbcTimeout } from "atoms/transactions";
import BigNumber from "bignumber.js";
import { getDefaultStore } from "jotai";
import { createIbcTransferMessage } from "lib/transactions";
import toml from "toml";
import {
AddressWithAsset,
IbcTransferTransactionData,
LocalnetToml,
TransferStep,
} from "types";
import { toBaseAmount } from "utils";
Expand Down Expand Up @@ -189,3 +191,8 @@ export const updateIbcTransactionStatus = async (
});
}
};

export const fetchLocalnetTomlConfig = async (): Promise<LocalnetToml> => {
const response = await fetch("/localnet-config.toml");
return toml.parse(await response.text()) as LocalnetToml;
};
2 changes: 1 addition & 1 deletion apps/namadillo/src/hooks/useRegistryFeatures.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
import { useAtom, useAtomValue } from "jotai";
import { useEffect } from "react";

const { VITE_PROXY } = process.env;
const { VITE_PROXY } = import.meta.env;

const namadaChainRegistryUrl =
VITE_PROXY ?
Expand Down
48 changes: 30 additions & 18 deletions apps/namadillo/src/registry/namadaAsset.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,33 @@
import { Asset } from "@chain-registry/types";
import { localnetConfigAtom } from "atoms/integrations/atoms";
import { getDefaultStore } from "jotai";
import namadaShieldedSvg from "./assets/namada-shielded.svg";

export const namadaAsset = {
name: "Namada",
base: "unam",
address: "tnam1qy440ynh9fwrx8aewjvvmu38zxqgukgc259fzp6h",
display: "nam",
symbol: "NAM",
denom_units: [
{
denom: "unam",
exponent: 0,
},
{
denom: "nam",
exponent: 6,
},
],
logo_URIs: { svg: namadaShieldedSvg },
} satisfies Asset;
const ADDRESS = "tnam1qxp7u2vmlgcrpn9j0ml7hrtr79g2w2fdesteh7tu";

// We can't return "satisfies Asset" from fn
// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
export const namadaAsset = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm removing this implementation and moving it to namada-chain-registry #1308

Do you think it could have some impact here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be fine I can fix conflict later

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, the conflict arrived 🙈 #1308

const store = getDefaultStore();
const config = store.get(localnetConfigAtom);
const address = config.data?.tokenAddress || ADDRESS;

return {
name: "Namada",
base: "unam",
address,
display: "nam",
symbol: "NAM",
denom_units: [
{
denom: "unam",
exponent: 0,
},
{
denom: "nam",
exponent: 6,
},
],
logo_URIs: { svg: namadaShieldedSvg },
} satisfies Asset;
};
5 changes: 5 additions & 0 deletions apps/namadillo/src/setupTests.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import "@testing-library/jest-dom";
import { atom } from "jotai";

jest.mock("atoms/integrations", () => ({
getRestApiAddressByIndex: jest.fn(),
getRpcByIndex: jest.fn(),
}));

jest.mock("atoms/integrations/atoms", () => ({
localnetConfigAtom: atom({ data: undefined }),
}));
6 changes: 6 additions & 0 deletions apps/namadillo/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -333,3 +333,9 @@ export type TransferTransactionData =

export type PartialTransferTransactionData = Partial<TransferTransactionData> &
Pick<TransferTransactionData, "type" | "chainId" | "asset">;

export type LocalnetToml = {
enabled: boolean;
chain_id: string;
token_address: string;
};
5 changes: 3 additions & 2 deletions apps/namadillo/src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,14 @@ const findDisplayUnit = (asset: Asset): AssetDenomUnit | undefined => {
};

const isNamAsset = (asset: Asset): boolean =>
asset.symbol === namadaAsset.symbol;
asset.symbol === namadaAsset().symbol;

export const toDisplayAmount = (
asset: Asset,
baseAmount: BigNumber
): BigNumber => {
if (isNamAsset(asset)) return baseAmount;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this won't work for NAM transferred from other chains. The indexer will return the balances in display denom for NAM from the current chain, but in base denom for NAM from other chains. But this check here will assume any token with symbol as NAM is in display denom.

I was wondering if this check should be outside the toDisplayAmount function anyway, since to me it seems like that logic is outside of the scope of the function. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I just also realised I think it's not going to work for displaying NAM on Cosmos chains either. The NAM balance we get from a Cosmos chain will be in base denom, but this check will stop it being converted to display denom.

Copy link
Collaborator Author

@mateuszjasiuk mateuszjasiuk Nov 26, 2024

Choose a reason for hiding this comment

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

yeah maybe moving this outside makes sense :D Just so I understand better, what do you mean by "NAM transferred from other chains." For NAM to arrive on other chain it first has to be sent from namada chain. So if we send NAM to cosmos and we send it back, will be it treated as different token than regular NAM?

Copy link
Collaborator

@emccorson emccorson Nov 26, 2024

Choose a reason for hiding this comment

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

The two cases I think won't work are:

  1. NAM that has been sent to a Cosmos chain (since the balance query for cosmjs returns amounts in base denom)
  2. NAM from something other than the current chain e.g. your current chain is housefire but you sent some NAM from internal devnet to housefire

In both cases, these should display properly as the Namada asset in Namadillo, but the denomation will be wrong in this PR because Namadillo thinks it's already in display denom but it's actually in base denom.

The case you're talking about where you send NAM from Namada to Cosmos and then back should work fine I think, because the indexer will still return the balance using the original tnam address and in display denom (as far as I know).

(Also, even though in case 2, the NAM from internal devnet displays as the Namada asset, it is a different thing from NAM from housefire, but we don't actually indicate that anywhere right now.)


const displayUnit = findDisplayUnit(asset);
if (!displayUnit) {
return baseAmount;
Expand All @@ -139,7 +141,6 @@ export const toBaseAmount = (
asset: Asset,
displayAmount: BigNumber
): BigNumber => {
if (isNamAsset(asset)) return displayAmount;
const displayUnit = findDisplayUnit(asset);
if (!displayUnit) {
return displayAmount;
Expand Down
Loading