Skip to content
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

Validator bonds contract first version #6

Merged
merged 6 commits into from
Dec 21, 2023
Merged

Conversation

ochaloup
Copy link
Contributor

PR contains the code for the raw on-chain validator bonds program + some unit tests.

SDK, CLI, tests and functionality verification will be incoming in future code changes.

@janlegner this should be in a shape we talked about in our discussions and hopefully it stands for a functional design where only some details need to be adjusted.

@ochaloup ochaloup requested a review from janlegner November 30, 2023 14:58
@janlegner
Copy link
Collaborator

Few notes to keep in mind as per our discussion:

  • readme with table of what stake_authority/withdraw_authority are at which stages
  • withdrawer_authority -> withdraw_authority
  • lockups - users can create arbitrary stake accounts (even with lockups), sdk must be prepared for that when showing total usable deposits
  • deactivated accounts may change delegation when merged (e.g. with maliciously crafted stake account), double-check that we are ready for this when merging/dealing with deactivated accounts

Copy link
Contributor Author

@ochaloup ochaloup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janlegner thanks a lot for the comments and the review.

Meanwhile I work on the base TS SDK calls and tests that I'm not putting to this PR and I add only changes particular to the contract itself.

  • I renamed deposit_bond to fund_bond (the way how fund_settlement is named) and with that withdraw_deposit to claim_withdraw_request (I want to have the withdraw_request to be used as term for init, cancel and a obtain action of the withdrawal)
  • I found an runtime issue with deserialization of vote account that was adjusted
  • I think to have parameterized the amount of minimum stake. It was 1 SOL as constant but with incoming changes to Solana it makes sense to me to be configurable in Config.
  • +1, reset stake means to activate the stake as well
  • I bit hesitate to change the term withdrawer authority to withdraw authority. But I don't say I'm against. I would like just to double check. My initial attempt was to be consistent with stake account naming (https://github.com/solana-labs/solana/blob/v1.17.10/sdk/program/src/stake/state.rs#L279). I can see that the bot uses rather the withdraw authority term. So just to double check on intentions and naming terminology.
  • that part with lockups is right and I'm aware that SDK needs to manage it. On rechecking this I changed the way how the lockup is checked. Custodian handling removed. It complicates the thing, it could be added later.
  • thanks for noting on the merging of the deactivated stake accounts. The merge permits only to merge when the staker authorities matches and when the delegation matches. I recheck the other commands and updated to be aware of the different stake accounts can be provided maliciously.

@janlegner
Copy link
Collaborator

* I bit hesitate to change the term `withdrawer authority` to `withdraw authority`. But I don't say I'm against. I would like just to double check. My initial attempt was to be consistent with stake account naming (https://github.com/solana-labs/solana/blob/v1.17.10/sdk/program/src/stake/state.rs#L279). I can see that the bot uses rather the `withdraw authority` term. So just to double check on intentions and naming terminology.

My bad, I thought the withdraw authority is consistent with stake program but i was wrong

Copy link
Collaborator

@janlegner janlegner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can merge this and continue work in consequent PRs

@ochaloup ochaloup merged commit 8e10b3b into main Dec 21, 2023
1 check passed
@ochaloup ochaloup mentioned this pull request Dec 21, 2023
@ochaloup ochaloup deleted the contract-first-version branch February 21, 2024 10:15
ochaloup added a commit that referenced this pull request May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants