-
Notifications
You must be signed in to change notification settings - Fork 2.3k
token-js: add JS helpers for interest bearing tokens' UIAmounts #7541
Conversation
preUpdateAverageRate: number; | ||
lastUpdateTimestamp: bigint; | ||
lastUpdateTimestamp: number; |
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 were changed because the buffer encoder does not work with bigints. numbers should be fine for timestamps and this doesn't seem to impact any other current usage
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.
Sorry, what do you mean about the buffer encoder not working? We use bigints in a lot of the extensions and they seem to work. I'd prefer to leave this as is
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.
Specifically if you use a bigint it has an issue when trying to encode this state due to this function that's in the buffer encoding library:
function divmodInt64(src) {
const hi32 = Math.floor(src / V2E32); // cannot mix bigint and numbers here
const lo32 = src - (hi32 * V2E32);
return { hi32, lo32 };
}
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 believe to fix this we'd have to fix the buffer encoding library. I took a look through all other state type definitions I could find and didn't see any other bigints
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.
Really great work! Mostly small things to fixup, but the logic is there and the tests look good
connection: Connection, | ||
mint: PublicKey, | ||
uiAmount: string, | ||
programId = TOKEN_PROGRAM_ID, |
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.
Up to you, but since this function is taking a mint address, we can just figure out the program id from the mint owner.
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.
yep good call. I'll update this
@@ -1,7 +1,10 @@ | |||
import Decimal from 'decimal.js'; |
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 likely missing something about JS math, but JS's native number
is a 64-bit float, just like Rust's f64
. Why do we need another dependency for it?
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.
floating point math in js is notoriously bad. As an example: 0.1 + 0.2 !== 0.3
(it actually equals 0.30000000000000004
). I wish I knew of a better way than importing a new dependency. Any suggestions?
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.
Rust has the same behavior, so maybe that's actually ok?
println!("{}", 0.1 + 0.2);
Gives
0.30000000000000004
export async function amountToUiAmountForMintWithoutSimulation( | ||
connection: Connection, | ||
mint: PublicKey, | ||
amount: 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.
We use bigint
in most places in the code to represent a Rust u64
-- can we do the same thing here?
*/ | ||
|
||
export function amountToUiAmountWithoutSimulation( | ||
amount: 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.
Can we use a bigint
instead like the rest of the package?
connection: Connection, | ||
mint: PublicKey, | ||
amount: string, | ||
programId = TOKEN_PROGRAM_ID, |
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.
Up to you, but since this takes the mint's public key, we can use the account owner instead of forcing people to pass this
if (programId.equals(TOKEN_PROGRAM_ID)) { | ||
return BigInt(uiAmountScaled.trunc().toString()); | ||
} |
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.
Same as with the other function, I think we can remove this
): Promise<bigint> { | ||
Decimal.set({ toExpPos: 24, toExpNeg: -24 }) | ||
const mintInfo = await getMint(connection, mint, 'confirmed', programId); | ||
const uiAmountScaled = new Decimal(uiAmount).mul(new Decimal(10).pow(mintInfo.decimals)); |
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.
nit: same as with the other function, this can get moved under if (!interestBearingMintConfigState)
return BigInt(uiAmountScaled.trunc().toString()); | ||
} | ||
|
||
const currentTime = Math.floor(Date.now() / 1000); // Convert to seconds |
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.
up to you: same here, can fetch the clock for the cluster time
preUpdateAverageRate: number; | ||
lastUpdateTimestamp: bigint; | ||
lastUpdateTimestamp: number; |
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.
Sorry, what do you mean about the buffer encoder not working? We use bigints in a lot of the extensions and they seem to work. I'd prefer to leave this as is
pnpm-lock.yaml
Outdated
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.
Can you revert any changes that aren't necessary for the PR? It seems like we're just adding sinon and decimal.js currently (although I hope we don't need the second one)
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.
Looking really close! Once the encoding bit is solved and these nits are fixed, it should be good to go
* @param r - The interest rate in basis points. | ||
* @returns The calculated exponent. | ||
*/ | ||
const calculateExponentForTimesAndRate = (t1: number, t2: number, r: number) => { |
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.
nit: we don't typically use arrow functions in this package -- can this be updated to a function(...)
?
* @returns A promise that resolves to the current timestamp in seconds. | ||
* @throws An error if the sysvar clock cannot be fetched or parsed. | ||
*/ | ||
const getSysvarClockTimestamp = async (connection: Connection): Promise<number> => { |
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.
nit: same here, let's avoid the arrow function
* @param connection Connection to use | ||
* @param mint Mint to use for calculations | ||
* @param amount Amount of tokens to be converted to Ui Amount | ||
* @param programId SPL Token program account (default: TOKEN_PROGRAM_ID) |
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.
nit: this param doesn't exist
* @param programId SPL Token program account (default: TOKEN_PROGRAM_ID) |
|
||
// Calculate post-update exponent | ||
// e^(currentRate * (currentTimestamp - lastUpdateTimestamp) / (SECONDS_PER_YEAR * ONE_IN_BASIS_POINTS)) | ||
const postUpdateExp = calculateExponentForTimesAndRate(lastUpdateTimestamp, Number(currentTimestamp), currentRate) |
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.
nit: Number
shouldn't be needed, unless I've missed something
const postUpdateExp = calculateExponentForTimesAndRate(lastUpdateTimestamp, Number(currentTimestamp), currentRate) | |
const postUpdateExp = calculateExponentForTimesAndRate(lastUpdateTimestamp, currentTimestamp, currentRate) |
const totalScale = preUpdateExp * postUpdateExp; | ||
|
||
// Calculate original principle by dividing the UI amount (principle + interest) by the total scale | ||
const originalPrinciple = uiAmountScaled / totalScale; |
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.
nit: If we want to use the term, it's principal
, not principle
https://www.merriam-webster.com/dictionary/principal
|
||
// Calculate original principle by dividing the UI amount (principle + interest) by the total scale | ||
const originalPrinciple = uiAmountScaled / totalScale; | ||
return BigInt(Math.floor(originalPrinciple)); |
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.
nit: is there a reason to use floor
over trunc
like with the other one?
* @param programId SPL Token program account (default: TOKEN_PROGRAM_ID) | ||
* |
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.
nit: since the param was removed
* @param programId SPL Token program account (default: TOKEN_PROGRAM_ID) | |
* |
throw new Error('Invalid program ID'); | ||
} | ||
|
||
const mintInfo = await getMint(connection, mint, 'confirmed', programId); |
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 causes a second network call to fetch the account again, so let's do unpackMint
like with the other function
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.
One last one, then we can merge!
Co-authored-by: Jon C <[email protected]>
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.
Thanks for all your patience and your work! Once it passes CI, I'll merge it in
…ogram-library into igitter/ibe-js-helpers
Pull request has been modified.
This adds four new helper functions to allow developers to better interact with interest bearing tokens.
UiAmounts are provided with the token's decimals as its precision and truncated instead of rounded to avoid rounding issues.
Amounts are processed in the base unit of the token (if the token has 2 decimals 1.23 is represented as 123)