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
stake-pool: add bindings to js #2806
stake-pool: add bindings to js #2806
Conversation
add new bindings | 'IncreaseValidatorStake' | 'DecreaseValidatorStake' | 'UpdateValidatorListBalance' | 'UpdateStakePoolBalance' | 'CleanupRemovedValidatorEntries'
add new bindings | 'IncreaseValidatorStake' | 'DecreaseValidatorStake' | 'UpdateValidatorListBalance' | 'UpdateStakePoolBalance' | 'CleanupRemovedValidatorEntries'
…program-library into stake_pool_bindings
Looks like there are still a few regressions introduced -- could you run this with npm version 7 so that package.lock stays on version 2, and also run the linting / formatting / building? |
is seems like prettier for some reason has different formatting options in the tests ... checking ... |
@AlexanderRay I just wanted to clarify a small thing: in the code, there are references to |
fixed |
# Conflicts: # stake-pool/js/package-lock.json # stake-pool/js/package.json
Again, great work on these bindings! I am receiving an error with the
I assume that Is there any way to create an |
Correction to my previous comment: it may be better to remove the detection step entirely and just have a helper function call the two instruction functions and concatenate the instructions. However, this is probably best to have in a separate, third-party library and not in the main bindings. |
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 meat of this looks really good! A few bits to clean up and fix -- let me know if you have any questions
@AlexanderRay small issue: after doing some testing on devnet, I discovered that This is one example of trying to withdraw lamports in a way that technically looks like it would be allowed given the total stake lamports in the validator account but doesn't honor the minimum amount of lamports that must be stored in a stake pool program validator account:
Here's another example, this time the amount requested for withdrawal is above the amount available in the validator account, and although there is another validator account that has enough lamports to make up the difference, the bindings still attempt to withdraw more from the first validator account than possible:
The bindings have previously dipped into multiple validator accounts and successfully withdrawn stakes from multiple validator accounts in a single transaction, so I know that can't be the issue. |
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.
A couple last bits and this should be good, pending @CMEONE 's issue
can you please check now if i works properly on your side |
Ok @AlexanderRay I will check if it works and let you know, also for instances where there are more instructions that can be fit into a transaction (like if it has to withdraw stake from multiple validator accounts), how should the instructions be split most optimally (or how should they be grouped in transactions if the unstake requires multiple transactions)? |
@AlexanderRay the minimum balance issue seems to be fixed for
The expected address |
The issue described above also seems to be the reason that CI is now failing.
|
@AlexanderRay I think the issue is that |
numerator: fee.denominator.sub(fee.numerator), | ||
denominator: fee.denominator, | ||
}; | ||
|
||
for (const type of ['preferred', 'active', 'transient', 'reserve']) { |
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 tried removing transient from this list, but then the the error said:
Program log: Error withdrawing from reserve: validator stake accounts have lamports available, please use those first.
Program log: Error: The lamports in the validator stake account is not equal to the minimum
Maybe the issue is way back in the findStakeProgramAddress
function?
I just don't understand why I'm getting the error |
stake-pool/js/src/utils/stake.ts
Outdated
continue; | ||
} | ||
|
||
let availableForWithdrawal = calcPoolTokensForDeposit(stakePool, lamports - minBalance); |
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.
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.
That makes sense, but when I tried modifying that line to let availableForWithdrawal = calcPoolTokensForDeposit(stakePool, type == 'transient' ? lamports - minBalance : lamports);
, I still got the same error Incorrect stake account address for vote <VOTE_ACCOUNT>, expected <ACTIVE_STAKE_ADDRESS>, received <ACTIVATING_TRANSIENT_STAKE_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.
From the logs, it seems like the issue is not just with not withdrawing everything possible, it's also with trying to withdraw from transient stake accounts before the validator account.
These are the accounts available in the pool on devnet:
- Validator 1 active
- Validator 2 active
- Validator 3 active
- Validator 3 transient
This is the order that the bindings chose for withdrawing stake:
- Validator 1 active
- Validator 3 transient
As you see, the bindings chose to try to withdraw from validator 3 transient before trying validator 3 active (which is where the error occurred).
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.
Also, when useReserve
is set to true, it tries withdrawing all the balance from the reserve even if there aren't enough lamports in reserve to cover it fully.
@CMEONE @joncinque please check, last commit should fix the issue |
@AlexanderRay Unfortunately, I am still experiencing the same two issues. When |
const stakeAccountRentExemption = await connection.getMinimumBalanceForRentExemption( | ||
StakeProgram.space, | ||
); | ||
|
||
const withdrawAuthority = await findWithdrawAuthorityProgramAddress( |
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.
A few lines below this one, there is the following code:
if (useReserve) {
withdrawAccounts.push({
stakeAddress: stakePool.account.data.reserveStake,
voteAddress: undefined,
poolAmount,
});
}
Won't this always fail if there is still stake in the validator accounts? In that case, I think it might be better to remove this part and pass useReserve
into the prepareWithdrawAccounts
method.
Then, in prepareWithdrawAccounts
, we can change this:
if (reserveStakeBalance > 0) {
accounts.push({
type: 'reserve',
stakeAddress: stakePool.reserveStake,
lamports: reserveStakeBalance,
});
}
to this:
if (useReserve && reserveStakeBalance > 0) {
accounts.push({
type: 'reserve',
stakeAddress: stakePool.reserveStake,
lamports: reserveStakeBalance,
});
}
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.
A few lines below this one, there is the following code:
if (useReserve) { withdrawAccounts.push({ stakeAddress: stakePool.account.data.reserveStake, voteAddress: undefined, poolAmount, }); }
Won't this always fail if there is still stake in the validator accounts? In that case, I think it might be better to remove this part and pass
useReserve
into theprepareWithdrawAccounts
method.Then, in
prepareWithdrawAccounts
, we can change this:if (reserveStakeBalance > 0) { accounts.push({ type: 'reserve', stakeAddress: stakePool.reserveStake, lamports: reserveStakeBalance, }); }
to this:
if (useReserve && reserveStakeBalance > 0) { accounts.push({ type: 'reserve', stakeAddress: stakePool.reserveStake, lamports: reserveStakeBalance, }); }
When processing withdrawals, the order of priority goes:
preferred withdraw validator stake account (if set)
validator stake accounts
transient stake accounts
reserve stake 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.
Right, that's what it should be. However, when useReserve
is set to true
, the order becomes:
reserve stake account
which will never work because: The lamports in the validator stake account is not equal to the minimum
The stake pool program won't allow for a stake withdrawal from reserve stake account until all the validator stake accounts have been fully withdrawn.
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.
argh ... checking .. anyway to withdraw from reserve account you should use withdrawSol
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.
Right of course, withdrawSol
is most optimal, useReserve
seems a bit out of place as a parameter and can probably be removed altogether.
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 I have a solution for the other bug (Incorrect stake account address for vote <VOTE_ACCOUNT>, expected <ACTIVE_STAKE_ADDRESS>, received <ACTIVATING_TRANSIENT_STAKE_ADDRESS>
), I'll add another comment once I test that it works (currently dealing with splitting transactions up since transaction sizes can get a bit large sometimes).
@CMEONE is your test pool on devnet? can you share your test environment with us? |
stake-pool/js/src/utils/stake.ts
Outdated
); | ||
if (!stakePool.stakeWithdrawalFee.denominator.isZero()) { | ||
for (const { stakeAddress, voteAddress, lamports } of filteredAccounts) { | ||
if (lamports <= minBalance) { |
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 this condition should be changed to (lamports <= minBalance && type == "transient")
because I think @joncinque was saying that the minimum balance part only applies to transient stake accounts.
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.
With this fix, everything seems to be working on devnet.
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 reserve part is still problematic, but from inspecting the issue, it seems like there's not much that can be done in the bindings themselves (seems to be an issue in the program itself). For instance, with completely empty validator stake accounts and all the transient stake set to the minimum balance, the program still returns:
Program log: Error withdrawing from reserve: validator stake accounts have lamports available, please use those first.
Program log: Error: The lamports in the validator stake account is not equal to the minimum
It seems like it might actually be impossible to withdraw stake from the reserve account even when all other accounts have been fully depleted! In that case, it's probably not worth spending too much time trying to fix that on the bindings side.
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 can confirm the reserve part does have a corner case, since it's impossible to totally drain a transient account. I think it's possible to relax that requirement, but I have to think through the implications.
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.
'use_reserve' functionality is just reimplemented bases on cli version
solana-program-library/stake-pool/cli/src/main.rs
Line 1373 in 3d92f8f
let withdraw_accounts = if use_reserve { |
@joncinque should I also remove it from cli or make it still sense there?
if (!stakePool.stakeWithdrawalFee.denominator.isZero()) { | ||
for (const { stakeAddress, voteAddress, lamports } of filteredAccounts) { | ||
if (lamports <= minBalance) { | ||
continue; |
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 change the above condition to (lamports <= minBalance && type == "transient")
as the minBalance
only applies to transient stake accounts. I believe that is all that needs to be fixed (besides useReserve
which is a larger issue related to reserve accounts in general).
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.
Thoroughly tested the following methods on devnet: getStakePoolAccount
, updateStakePool
, depositSol
, withdrawSol
, withdrawStake
, stakePoolInfo
If @joncinque thinks everything looks good, I think this PR can be merged (and pushed to NPM with a minor version bump), and we can revisit the reserve issues in another PR (since that's a larger issue that involves the program itself).
I can't wait to launch BlazeStake on mainnet-beta soon, which would not have been possible without both of you. Thank you so much for your work on these bindings (and for persisting despite all the errors)!
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 your patience, looks great!
add new bindings
add new function
@joncinque