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

Integrate hooks #4

Merged
merged 29 commits into from
Sep 20, 2023
Merged

Integrate hooks #4

merged 29 commits into from
Sep 20, 2023

Conversation

amityadav0
Copy link

@amityadav0 amityadav0 commented Sep 15, 2023

  • Add hooks for HoM, CoA, TC
  • Add tests

astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
@amityadav0 amityadav0 marked this pull request as ready for review September 17, 2023 18:40
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
Copy link

@sczembor sczembor left a comment

Choose a reason for hiding this comment

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

left few comments. I think we should unit test the methods that were added. If the panics are returned in the correct scenarios

Copy link

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

  • documentation should be updated about the new roles.
  • need to add a unit test with a whole flow:
    • normal member makes proposal
    • other members vote
    • voting_body vetos the proposal
    • proposal is removed, and people can't vote

astra/src/bounties.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Show resolved Hide resolved
astra/src/lib.rs Show resolved Hide resolved
astra/src/lib.rs Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/policy.rs Outdated Show resolved Hide resolved
astra/src/policy.rs Outdated Show resolved Hide resolved
amityadav0 and others added 6 commits September 19, 2023 16:20
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: sczembor <43810037+sczembor@users.noreply.github.com>
@robert-zaremba
Copy link

robert-zaremba commented Sep 19, 2023

The idea I am thinkging about is integrate the permissions in the existing role based model. We will need to add two new variants to the Action: Veto and Dissolve.

In CoA, we DON'T NEED to define new ProposalKinds (eg no need for Veto, Dismiss). CoA can make a normal FunctionCall proposal to call hom.veto.

Once the proposal succeedes, this will be sent to HoM DAO:

hom.veto(prop_id: u64, reason: String) {
   1. check if caller has "Veto" permisson
   2. check if the proposal is active (or in cooldown - can be in separater PR)
   3. remove proposal and return bond
   4. emit events (separate PR -> task already created)
   5. // maybe I forogt about something
}

In the HoM, the "CoA" role can be defined as:

RolePermission {
     name: "CoA",
     kind: RoleKind::Group(vec![coa_address]),
     permissions: {"Veto", "Dissolve"},  // Need to extend Action enum by adding Veto...
     vote_policy: {}, // empty set - no voting
}

Note: that we use "Veto" here, not ":Veto", because this is not an acto on a proposal. So we will need new function: policy.can_execute_hook, which will only check permissions.contains(&action.to_policy_label())

Note2: later we will need to add one more permission: "Dismiss"

@robert-zaremba
Copy link

robert-zaremba commented Sep 19, 2023

If we want to extend the RoleKind then we can add RoleKind::Individual(AccountId). But I am not sure if it is needed. And certainly it should not be skipped when doing vote calculation , but in permissions level: https://github.com/near-ndc/astra/blob/8a5f38ba91/astra/src/policy.rs#L329

@amityadav0
Copy link
Author

Okay, thanks for the detailed explanation, I have updated the PR with the changes

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

nearly there

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/test_utils.rs Outdated Show resolved Hide resolved
amityadav0 and others added 2 commits September 20, 2023 11:01
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
amityadav0 and others added 2 commits September 20, 2023 14:04
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/lib.rs Outdated Show resolved Hide resolved
astra/src/policy.rs Outdated Show resolved Hide resolved
astra/src/types.rs Outdated Show resolved Hide resolved
astra/src/proposals.rs Show resolved Hide resolved
@amityadav0 amityadav0 merged commit c5fac1d into master Sep 20, 2023
1 check passed
@amityadav0 amityadav0 deleted the hooks branch September 20, 2023 10:54
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