-
Notifications
You must be signed in to change notification settings - Fork 44
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
added solido.js library to interact with solido program #478
Conversation
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 looked at the first few files but I haven’t gotten to the meat of it yet. Is there a tool that I can run to generate API reference docs? (Something like what rustdoc
is for Rust?) I see https://solana-labs.github.io/solana-web3.js/ is generated by Typedoc, is that the one that the standard one? Do we need to configure anything to set it up?
### Run tests watch script | ||
``` | ||
$ npm run watch:test | ||
``` |
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.
What’s the difference between watch:ts
and watch:test
? One just typechecks the TypeScript and the other one also runs the tests?
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.
watch:ts
compile the ts files on every file change
watch:test
runs the test on every file change
|
||
### Javascript | ||
```js | ||
const SolidoJS = require("@chorusone/solido.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.
Is it common to use the JS
suffix even in names in Javascript?
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 really, but I just used it in the example here
### Javascript | ||
```js | ||
const SolidoJS = require("@chorusone/solido.js"); | ||
const snapshot = SolidoJS.getSnapshot(...params); |
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 should give a full example that works. I think it needs a connection and also the SolidoJS.MAINNET_PROGRAM_ADDRESSES
, right?
{ | ||
"name": "@chorusone/solido.js", | ||
"version": "1.0.0", | ||
"description": "Typescript SDK for interacting with Solido Program", |
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.
Shall we say “for interacting with Lido for Solana (Solido)” to make it more discoverable?
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.
Sure we should do this
stSolMintAddress: new PublicKey( | ||
'7dHbWXmci3dT8UFYWYZweBLXgycu7Y3iL6trKn1Y7ARj' | ||
), | ||
}; |
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 cross-checked these addresses against https://docs.solana.lido.fi/deployments and I can confirm that they are correct.
|
||
export const findAuthorityProgramAddress = async ( | ||
programAddresses: ProgramAddresses, | ||
additionalSeedString: 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 this be more strictly typed? AFAICT the only relevant strings are reserve_account
and mint_authority
. Are rewards_withdraw_authority
and sol_reserve
also valid?
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.
Yeah good idea, we can add all the available options to it like "reserve_account" | "mint_authority" ...
To help the user(editor suggestions) and to make it more strictly typed
Looks great. A couple questions,
|
There is a separation between constructing instructions and transactions (which are lists of instructions), and querying the on-chain state. I think it’s nicer to keep the functions that build instructions pure, without side effects. If all instruction builders query the chain state themselves, this can result in torn reads, and observing inconsistent states, because they all query the chain at different times, and this makes it difficult to compose different instructions into one transaction. We should make a convenience function though that returns all instructions that you need to do a deposit, and that can then include the instruction to create the ATA if it doesn’t exist yet.
It depends on the RPC node configuration how many accounts you can query in a single
To be able to determine which validator to withdraw from, we only need the Solido state. But to be able to determine the maximum amount to withdraw, we also need the stake account. However, if we first fetch the Solido state, and then fetch the stake account, you run into the torn reads again, and then we build transactions based on an inconsistent view of the chain. Right now the only way to get a consistent view through the Solana RPC, is to use I don’t think it will be a big issue though; we should be able to get all the data we need with just two RPC calls, and that will serve us up to ~100 validators, and there is a lot we can optimize at that point to scale further if we need to. (For example, only fetch the top n stake accounts, because we don’t expect them to change quickly anyway. Or don’t look at the stake accounts at all and just set a slightly lower per-instruction withdraw limit.) |
@hritique I ran |
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 reviewed a few more files, but I haven’t gotten through all of it yet.
'mint_authority' | ||
); | ||
|
||
const keys = [ |
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 here, can you a comment to reference
solido/program/src/instruction.rs
Lines 167 to 205 in e7f493e
accounts_struct! { | |
DepositAccountsMeta, DepositAccountsInfo { | |
pub lido { | |
is_signer: false, | |
// Needs to be writable for us to update the metrics. | |
is_writable: true, | |
}, | |
pub user { | |
is_signer: true, | |
// Is writable due to transfer (system_instruction::transfer) from user to | |
// reserve_account | |
is_writable: true, | |
}, | |
pub recipient { | |
is_signer: false, | |
// Is writable due to mint to (spl_token::instruction::mint_to) recipient from | |
// st_sol_mint | |
is_writable: true, | |
}, | |
pub st_sol_mint { | |
is_signer: false, | |
// Is writable due to mint to (spl_token::instruction::mint_to) recipient from | |
// st_sol_mint | |
is_writable: true, | |
}, | |
pub reserve_account { | |
is_signer: false, | |
// Is writable due to transfer (system_instruction::transfer) from user to | |
// reserve_account | |
is_writable: true, | |
}, | |
pub mint_authority { | |
is_signer: false, | |
is_writable: false, | |
}, | |
const spl_token = spl_token::id(), | |
const system_program = system_program::id(), | |
} | |
} |
data, | ||
programId: programAddresses.solidoProgramId, | ||
}); | ||
}; |
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.
👍 getDepositInstruction
looks good to me, both implementation-wise and API-wise. It makes sense that it doesn’t take a snapshot as input because deposit does not depend on the snapshot.
Organization-wise, and for discoverability, it may be better later to put it in the same place as the withdraw instruction. (Maybe withdraw can be a method on snapshot, and deposit a static method?) But we can worry about making that nicer later.
} from '@solana/spl-token'; | ||
import { PublicKey, TransactionInstruction } from '@solana/web3.js'; | ||
|
||
export const getATAInitializeInstruction = async ( |
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, I prefer to capitalize abbreviations like regular words because I find JsonRpcXmlHttpRequestBackend
more readable than JSONRPCXMLHTTPRequestBackend
. Also, it may not be obvious what an “ATA” is, although
getInitializeAssociatedTokenAccountInstruction
is quite long ... I have no strong opinion on this, but I think I would just write it out fully. If you are new to the Solana ecosystem and you read code that calls getAtaInitializeInstruction
you have no clue what it does and it may be difficult to find. If it says “associated token account”, then you at least know that it’s a token account, and searching for “associated token account” is more obvious than just “ATA”.
js/src/instructions/withdraw.ts
Outdated
|
||
export const getWithdrawInstruction = async ( | ||
snapshot: Snapshot, | ||
programAddresses: ProgramAddresses, |
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.
Let’s make ProgramAddresses
a field of Snapshot
and initialize it when we create the snapshot. That’s one less thing for callers to worry about, and also makes it impossible to mix a snapshot created from say testnet ProgramAddresses
with mainnet ProgramAddresses
here.
getHeaviestValidatorStakeAccount(snapshot); | ||
|
||
if (amount.lamports.lte(snapshot.stakeAccountRentExemptionBalance.lamports)) { | ||
throw new Error('Amount must be greater than the rent exemption balance'); |
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.
Where does Error
come from? Is it something that we define, or is it a general TypeScript error type? If it’s the latter, does it make sense to define our own error type so callers can catch just these errors without accidentally swallowing problems that happen elsewhere?
We should also document in the doc comment that the function may throw this type.
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.
Yes, we'll add a list of possible errors so that users can catch and handle them better
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 add those to the docs then?
js/src/instructions/withdraw.ts
Outdated
|
||
const maxWithdrawAmount = heaviestValidatorStakeAccount.balance.lamports | ||
.div(new BN(10)) | ||
.add(new BN(10)); |
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.
The max withdraw amount is 1/10 of the balance + 10 SOL, not 10 Lamports! This should be 10_000_000_000
. (Does TypeScript have numeric underscores? Or is there a LAMPORTS_PER_SOL
constant somewhere?)
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.
Ohhh yes, thanks for pointing this. Would have missed this
js/src/instructions/withdraw.ts
Outdated
throw new Error('Amount must be greater than the rent exemption balance'); | ||
} | ||
|
||
const maxWithdrawAmount = heaviestValidatorStakeAccount.balance.lamports |
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.
To clarify that the amount is in SOL, we can wrap the expression on the right-hand side in Lamports()
. These wrapper types are not just useful for users, also for us!
js/src/instructions/withdraw.ts
Outdated
stSolAccountOwnerAddress: PublicKey, | ||
senderStSolAccountAddress: PublicKey, | ||
recipientStakeAccountAddress: PublicKey, | ||
amount: Lamports |
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.
Amount is in StLamports
, not in Lamports
!
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.
Yes, stumbled upon this today. Will fix this soon
js/src/instructions/withdraw.ts
Outdated
.div(new BN(10)) | ||
.add(new BN(10)); | ||
|
||
if (amount.lamports.gte(maxWithdrawAmount)) { |
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 is mixing things in different units (and that is where the different wrapper types come in, to prevent this). Is it possible to overload the <=
operator in TypeScript? Then we can make sure that for Lamports
on the left-hand side, the right-hand side must also be Lamports
, and the compiler will reject amount < maxWithdrawAmount
with a type error, because amount: StLamports
but maxWithdrawAmount: Lamports
. This way we can leverage the type system to prevent bugs :-)
To fix the bug, we need to convert the stSOL amount to SOL, according to the exchange rate in the snapshot.
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.
Ahhh yes, that make sense. Will try to fix 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.
I reviewed a few more files, only a few left to go!
js/src/instructions/deposit.ts
Outdated
programAddresses: ProgramAddresses, | ||
amount: Lamports | ||
) => { | ||
// Reference: Deposit instruction at https://github.com/ChorusOne/solido/blob/main/program/src/instruction.rs#L37-L43 |
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.
Please use a permalink to a particular commit (you can press y on GitHub when viewing a page to make it substitute main
for the commit you are looking at). Without this, as soon as we push something to main
and insert a line somewhere, this link will now point to the wrong lines.
js/src/instructions/deposit.ts
Outdated
/** | ||
* Generates the instructions to stake SOL in the Solido Program | ||
* @param senderAddress Address of the sender | ||
* @param recipientStSolAddress Address of the recipient stSOL Account |
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.
* @param recipientStSolAddress Address of the recipient stSOL Account | |
* @param recipientStSolAddress Address of the recipient stSOL SPL token account |
js/src/instructions/deposit.ts
Outdated
recipientStSolAddress: PublicKey, | ||
programAddresses: ProgramAddresses, | ||
amount: Lamports | ||
) => { |
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.
Don’t we need to declare the return type as TransactionInstruction
anywhere?
js/src/instructions/utils.ts
Outdated
/** | ||
* Generates the instruction to create the Associated Token Account for the given mint address | ||
* @param mintAddress Mint address of the token | ||
* @param ownerAddress Address of the owner of the token |
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.
* @param ownerAddress Address of the owner of the token | |
* @param ownerAddress Address of the owner of the token account |
js/src/instructions/withdraw.ts
Outdated
/** | ||
* Generates the instructions to unstake from the Solido Program | ||
* @param snapshot Snapshot of the Solido stats | ||
* @param senderStSolAccountOwnerAddress Address of the owner of the sender's stSOL Account |
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.
* @param senderStSolAccountOwnerAddress Address of the owner of the sender's stSOL Account | |
* @param senderStSolAccountOwnerAddress Address of the owner of the sender's stSOL SPL token account |
js/src/snapshot.ts
Outdated
export const getSnapshot = async ( | ||
connection: Connection, | ||
solidoInstanceAccountInfo: AccountInfo<Buffer>, | ||
reserveAccountInfo: AccountInfo<Buffer>, |
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.
What are these AccountInfo
arguments, why do we need to pass them in? The connection + programAddresses should be sufficient to construct the snapshot. If we need any additional accounts, we should get them as part of the getMultipleAccounts
.
js/src/snapshot.ts
Outdated
reserveAccountInfo: AccountInfo<Buffer>, | ||
programAddresses: ProgramAddresses | ||
): Promise<Snapshot> => { | ||
const solido = getSolido(solidoInstanceAccountInfo.data); |
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.
Now we still do a torn read, the getMultipleAccounts
later on can read from a later slot than where we got the solido
account from, and they may have changed in the meantime. To get a consistent view, we have to re-read the solido
account as part of the getMultipleAccounts
. One way to do this is in a loop:
- Start with just the Solido account, do a
getMultipleAccounts
, put the results in a dictionary keyed on accounts. - Start parsing the Solido state and the other accounts, but instead of calling the RPC, read from the dictionary.
- If an account is not present in the dictionary, add that address to the list of accounts to retrieve, and start again. To be more efficient, we can add all validator stake accounts to the list immediately. Then we should only have to do two reads (the first one to get the Solido instance, and the second one that gets everything we need). In the extremely unlucky case where you run this just as we add a validator, it will have to do three iterations.
js/src/snapshot.ts
Outdated
|
||
const { | ||
value: { amount }, | ||
} = await connection.getTokenSupply(programAddresses.stSolMintAddress); |
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 can’t use the getTokenSupply
if we want to avoid torn reads. It might happen that we read the Solido account, and we find that the stake accounts together hold 100 SOL (and 100 stSOL is minted for that, but that is not stored in the Solido instance). Then somebody withdraws, now there is only 50 SOL left and 50 stSOL minted. Then we call getTokenSupply
, and we find 50 stSOL. Now the snapshot claims to have 100 SOL but only 50 stSOL. But that’s not true, and even worse, there never even was a time at which it was true!
To avoid this, we have to use getMultipleAccounts
. That’s the only tool the Solana RPC gives us to avoid torn reads. We can include the stSOL mint in the accounts to query, and then use encoding: 'jsonParsed
. It returns data of this shape for SPL token mints:
"value": [
{
"data": {
"parsed": {
"info": {
"decimals": 9,
"freezeAuthority": null,
"isInitialized": true,
"mintAuthority": "8kRRsKezwXS21beVDcAoTmih1XbyFnEAMXXiGXz6J3Jz",
"supply": "1367470251242903"
},
"type": "mint"
},
"program": "spl-token",
"space": 82
},
"executable": false,
"lamports": 1461600,
"owner": "TokenkegQfeZyiNwAJbNbGKPFXCWuBvf9Ss623VQ5DA",
"rentEpoch": 268
}
]
(If you want to be really strict, we also need to avoid connection.getMinimumBalanceForRentExemption
, because it performs a separate read, and read and parse SYSVAR_RENT
instead. But the rent is unlikely to change, so I can live with that being a separate read. I expect it would be faster to do everything in a single getMultipleAccounts
read though, it saves a network round trip.)
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 did a first pass over all files now!
js/src/tests/data/snapshot.ts
Outdated
exchange_rate: { | ||
computed_in_epoch: new BN('0100', 'hex'), | ||
st_sol_supply: new BN('35e06bf954dd5', 'hex'), | ||
sol_balance: new BN('36a84a87b4459', 'hex'), |
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 is fine, but it may be more readable to just use base 10 here, i.e.
computed_in_epoch: new BN(256),
st_sol_supply: new BN(947_808_007_179_733),
sol_balance: new BN(961_542_925_010_009),
Or is this data generated and this is the default way in which it’s printed? If so, please add a comment to explain how to re-generate the data if needed.
js/src/utils.ts
Outdated
}; | ||
|
||
/** | ||
* Get the stake account that has the most amount of SOL staked(heaviest) |
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.
* Get the stake account that has the most amount of SOL staked(heaviest) | |
* Get the stake account that has the most amount of SOL staked (heaviest) |
js/src/utils.ts
Outdated
export const getHeaviestValidatorStakeAccount = ( | ||
snapshot: Snapshot | ||
): Snapshot['validatorsStakeAccounts'][0] => { | ||
const sortedValidatorStakeAccounts = snapshot.validatorsStakeAccounts.sort( |
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.
To get the validator with the most stake, we don’t need to sort (which has O(n log n) complexity, where n is the number of validators.) A single pass is sufficient. (Which has O(n) complexity.)
let heaviestStakeAccount = snapshot.validatorsStakeAccounts[0];
for (stakeAccount in snapshot.validatorsStakeAccounts) {
if (stakeAccount.balance > heaviestStakeAccount.balance) {
heaviestStakeAccount = stakeAccount;
}
}
return heaviestStakeAccount;
(Assuming we can overload operator>
for Lamports
.)
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 have a few small comments, but aside from those, let’s get this merged indeed. We can iterate on it from here, and we still need to test thoroughly either way before making a public release.
js/src/stats.ts
Outdated
}; | ||
|
||
/** | ||
* Get the number of all the token accounts exist for the token specified by the mint address | ||
* Get the number of token accounts that exist for the token specified by the mint address |
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.
* Get the number of token accounts that exist for the token specified by the mint address | |
* Get the number of token accounts that exist for the token specified by the mint address |
js/src/utils.ts
Outdated
* @param snapshot Snapshot of the Solido stats | ||
* @param amount SOL to exchange | ||
*/ | ||
export const exchangeSol = (snapshot: Snapshot, amount: Lamports) => { |
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 should add StLamports
as the return type.
js/src/utils.ts
Outdated
* @param snapshot Snapshot of the Solido stats | ||
* @param amount stSOL to exchange | ||
*/ | ||
export const exchangeStSol = (snapshot: Snapshot, amount: StLamports) => { |
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 should add Lamports
as the return type.
js/src/utils.ts
Outdated
export const exchangeSol = (snapshot: Snapshot, amount: Lamports) => { | ||
const exchangeRate = getExchangeRate(snapshot); | ||
|
||
return new StLamports(amount.lamports.toNumber() / exchangeRate); |
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 can be more precise here by using
(amount.lamports * snapshot.solido.exchange_rate.st_sol_supply).toNumber() / snapshot.solido.exchange_rate.sol_balance.toNumber();
This way the conversion to float doesn’t happen until after multiplying the two numbers, and we don’t divide by a quotient.
Also, if st_sol_supply
is zero, we should return the input amount. This does not happen on mainnet because the instance now has a nonzero balance, but for a fresh instance deployed to devnet it might. (This should avoid division by zero: if sol_balance
is zero, then for sure we can’t have any stSOL unless we have a bug. So if the stSOL supply is nonzero, it implies that sol_balance
is also nonzero.)
js/src/utils.ts
Outdated
export const exchangeStSol = (snapshot: Snapshot, amount: StLamports) => { | ||
const exchangeRate = getExchangeRate(snapshot); | ||
|
||
return new Lamports(amount.stLamports.toNumber() * exchangeRate); |
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 can be more precise here by using
(amount.stLamports * snapshot.solido.exchange_rate.sol_balance).toNumber() / snapshot.solido.exchange_rate.st_sol_supply.toNumber();
This way the conversion to float doesn’t happen until after multiplying the two numbers, and we don’t divide by a quotient.
|
||
let rawString = util.inspect(snapshot, true, 10, false); | ||
|
||
// Replace the ugly <BN: ...> public keys with readable ones |
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.
👍 Nice, this makes the tests easier to read indeed.
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!
const solReceived = exchangeStSol(snapshotDump, stSolToExchange); | ||
|
||
expect(solReceived.lamports.toString()).toBe('1000000000'); | ||
}); |
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 adding a test.
No description provided.