-
Notifications
You must be signed in to change notification settings - Fork 107
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,3 +8,4 @@ | |
/test-results/ | ||
/playwright-report/ | ||
/playwright/.cache/ | ||
/public/localnet-config.toml |
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 = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm removing this implementation and moving it to Do you think it could have some impact here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be fine I can fix conflict later There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; |
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 }), | ||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I was wondering if this check should be outside the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The two cases I think won't work are:
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; | ||
|
@@ -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; | ||
|
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 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 againstsymbol
orbase
instead?