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

Feature/positions2 WIP #6

Merged
merged 21 commits into from
Mar 8, 2022
Merged

Feature/positions2 WIP #6

merged 21 commits into from
Mar 8, 2022

Conversation

guibescos
Copy link
Contributor

@guibescos guibescos commented Feb 25, 2022

Goals :

  • Withdraw instruction
  • Close instruction
  • Revise instruction
  • Further tests

@guibescos guibescos requested a review from jayantk March 1, 2022 22:01
@guibescos
Copy link
Contributor Author

Some more meat on the menu:
staking/app/ contains some stuff for setting up a validator and solana accounts for UI dev work

Copy link
Collaborator

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

cool lots of good stuff in here. Left you a bunch of comments. I'd like to look at this again once you've addressed them.

Also, since this PR is so large, please make sure you don't break the commit history as you respond to the comments. (As long as you don't git push -f, the commit history should be fine.)

staking/app/Anchor.toml Show resolved Hide resolved
@@ -0,0 +1 @@
*.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe ignore the entire keypairs/ directory? This directory only exists for testing purposes right?

});

it("alice and bob get staking accounts", async () => {
for (let user of [
Copy link
Collaborator

Choose a reason for hiding this comment

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

(comment for the future) would be nice to make a typescript API for the staking program to wrap up these transactions into easy-to-use methods.

Definitely do not make this change as part of this PR.

staking/programs/staking/src/context.rs Show resolved Hide resolved
staking/programs/staking/src/error.rs Outdated Show resolved Hide resolved
staking/tests/staking.ts Show resolved Hide resolved
staking/tests/utils/constant.ts Show resolved Hide resolved
if (err instanceof ProgramError) {
assert.equal(
parseErrorMessage(err, idlErrors),
error
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor thing: this would be a nicer method to use if you made error a regex and had this check do a match against the string. This would help if there were error messages that got populated with parameters.

staking/programs/staking/src/utils/risk.rs Show resolved Hide resolved
staking/programs/staking/src/lib.rs Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Mar 7, 2022

❌ Deploy Preview for competent-kirch-7c1110 failed.

🔨 Explore the source changes: a4e0829

🔍 Inspect the deploy log: https://app.netlify.com/sites/competent-kirch-7c1110/deploys/6226e2ffb6e85000073e1c0f

Copy link
Collaborator

@jayantk jayantk left a comment

Choose a reason for hiding this comment

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

nicely done

for i in 0..MAX_POSITIONS {
if stake_account_positions.positions[i].is_some() {
let position = stake_account_positions.positions[i].unwrap();
match position.get_current_position(current_epoch, config.unlocking_duration)? {
PositionState::LOCKED => {
if position.product.is_none() {
voter_weight += position.amount;
if position.product.is_none() && position.publisher.is_none() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you may want to add a function like position.is_voting() for this particular check, because you're going to use it in a bunch of places.

* w <= floor((RISK_THRESH * vested balance - sum exposure_i)/RISK_THRESH)
* which implies the actual inequality.
* The maximum value for w is then just the minimum of all the RHS of all the inequalities.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 excellent comment

@guibescos guibescos merged commit b6c5f29 into master Mar 8, 2022
@guibescos guibescos deleted the feature/positions2 branch March 10, 2022 05:20
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.

3 participants