-
Notifications
You must be signed in to change notification settings - Fork 233
DEVREL-833 check Solana local decimals when creating OFT #1763
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
base: main
Are you sure you want to change the base?
Conversation
|
will be updating this PR to remove the concept of 'max recommended local decimals' and just prompt for confirmation every single time to ensure the dev is aware of the max total supply given the local decimals value |
|
updated |
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.
more documentation on the maxSupplyHuman part would help
the rest seems good
|
@shankars99 pushed changes to address your feedback |
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 = 1 is billion and 1000 * billion is trillion which is i = 0 right?
Yes, that's correct. But also yeah this part is buggy as it doesn't turn 1000M into 1B (or 1000K into 1M). Will fix.
Im thinking we can do something like
export function formatAmount(rawAmt: bigint, maxDisplayDecimals = 1): string {
const maxAmt = localDecimalsToMaxSupply(Number(rawAmt))
if (!Number.isInteger(maxDisplayDecimals) || maxDisplayDecimals < 0 || maxDisplayDecimals > 6) {
throw new Error('precision must be an integer between 0 and 6')
}
// Find the appropriate unit (largest unit that maxAmt is >= to)
const i = UNITS.findIndex((unit) => maxAmt >= unit.base)
if (i !== -1) {
const { base, suffix } = UNITS[i]
const pow = 10n ** BigInt(maxDisplayDecimals)
const scaled = (maxAmt * pow + base / 2n) / base /// half up
const quotient = scaled / pow
const rem = scaled % pow
const decimals =
maxDisplayDecimals === 0 ? '' : rem.toString().padStart(maxDisplayDecimals, '0').replace(/0+$/, '')
return `${quotient}${decimals ? `.${decimals}` : ''}${suffix}`
}
return maxAmt.toString()
}
which i think is a lot cleaner? not sure if it has the same issue that you mentioned but it could be a good starting point
|
@shankars99 I think for debtools, correctness should be sufficient. I don't really want to spend too much time on optimizing implementations. For correctness, I've added unit tests. You can check them out and see if any cases are missed. |
|
not really optimizations but it makes it easier to port these tooling to the new one. the new monorepo internal would not have all of these scripts that we have and its upto us to port them over. we also do not know when devtools will be out on public launch. its safer and better to keep our current implementations the best they can be even if it takes some extra time to compensate for a doomsday case |
|
This formatter shouldnt show rounded numbers. "... Solana OFT token will be 1.8B tokens..." This is misleading and not true, should simply show the number formatted with decimals and commas to the actual maximum value. |
| { base: BigInt(1000), suffix: 'K' }, | ||
| ] | ||
|
|
||
| export function formatTokenAmount(whole: bigint, maxDisplayDecimals = 1): string { |
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.
Why not use a builtin:
export const formatTokenAmount = (whole: bigint, maxDecimals = 1): string =>
new Intl.NumberFormat('en-US', {
notation: 'compact',
maximumFractionDigits: maxDecimals,
roundingMode: 'floor'
}).format(Number(whole))
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.
updated to use this suggestion
|
|
||
| it('throws on invalid precision values', () => { | ||
| expect(() => formatTokenAmount(BigInt(1_000), -1 as unknown as number)).toThrow() | ||
| expect(() => formatTokenAmount(BigInt(1_000), 7)).toThrow() |
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.
Why should this throw? if I want to round to 7 decimals of precision, and it doesn't exist in that numeration, just dont include them?
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.
amended per your suggestion.
| const mintPDA = await getMint(connection, new PublicKey(mintStr), undefined, new PublicKey(tokenProgramStr)) | ||
| const mintDecimals = mintPDA.decimals | ||
|
|
||
| const maxSupply = formatTokenAmount(localDecimalsToMaxSupply(mintDecimals)) |
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.
This isnt the MaxSupply, its the maxSupplyFloor, or maxSupplyRounded etc.
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.
These lines have now been amended to
const maxSupplyRaw = localDecimalsToMaxWholeTokens(decimals)
const maxSupplyFormatted = new Intl.NumberFormat('en-US').format(maxSupplyRaw)
const maxSupplyCompacted = formatTokenAmountCompact(maxSupplyRaw)
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.
@TRileySchwarz can you confirm whether this thread can be resolved?
examples/lzapp-migration/test/scripts/solanaUtils.script.test.ts
Outdated
Show resolved
Hide resolved
| // EOF: Validate combination of parameters | ||
|
|
||
| const maxSupplyRaw = localDecimalsToMaxWholeTokens(decimals) | ||
| const maxSupplyFormatted = new Intl.NumberFormat('en-US').format(maxSupplyRaw) |
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 would probably just have this formateTokenAmounts return all these values, so its not expensive or inefficient to always do the calcaultions, and that way there is one helper vs. multiple
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'm assuming you meant there should be one helper to serve these 2 lines:
const maxSupplyFormatted = new Intl.NumberFormat('en-US').format(maxSupplyRaw)
const maxSupplyCompacted = formatTokenAmountCompact(maxSupplyRaw)
rather than absorbing what localDecimalsToMaxWholeTokens does into formateTokenAmounts as I think these two functions do 2 separate things
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.
pushed:
export function formatTokenAmount(rawAmount: bigint, maxDisplayDecimals = 1): { full: string; compact: string } {
if (!Number.isInteger(maxDisplayDecimals) || maxDisplayDecimals < 0) {
throw new Error('maxDisplayDecimals must be an integer between 0 and 6')
}
const full = new Intl.NumberFormat('en-US').format(rawAmount)
const compact = new Intl.NumberFormat('en-US', {
notation: 'compact',
maximumFractionDigits: maxDisplayDecimals,
roundingMode: 'floor',
}).format(Number(rawAmount))
return {
full,
compact,
}
}
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.
@St0rmBr3w had to dismiss your review to make this change, do review again when you can 🙏
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.
LGTM!
| export { localDecimalsToMaxWholeTokens } from '@layerzerolabs/devtools-solana' | ||
| export { formatTokenAmount } from '@layerzerolabs/devtools' |
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.
why not just import it in createOFT and createOFTAdapter
| const maxSupplyStatement = `You have chosen ${mintDecimals} local decimals. The maximum supply of your Solana OFT token will be ${maxSupplyFormatted.full} (~${maxSupplyFormatted.compact}).\n` | ||
| const confirmMaxSupply = await promptToContinue(maxSupplyStatement) | ||
| if (!confirmMaxSupply) { | ||
| return | ||
| } |
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.
do adapters on solana define the max supply?
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.
Not the adapters themselves, but figured it would be good to point out to the partner dev what the max supply would be given the Solana token (Mint Account) that they supply to this command.
Changes
localDecimalsToMaxSupply(Solana) andformatTokenAmountCompactinpackages(added unit tests)createOFTandcreateOFTAdapterscripts inoft-solanaandlzapp-migration: added prompt to confirm that states what max supply will be based on local decimalsExample runs
Testing
pnpm hardhat lz:oft:solana:create --eid 40168 --program-id <PROGRAM_ID> --only-oft-store true --local-decimals 7and view the confirmation prompt.--local-decimalsto see the varying max supplyFor ease of testing you can use
export PROGRAM_ID= 9j9Wp46t7GyDSU3NVV8AmYMdXZHoiZmAzVuDSVDY2jy