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

Governance Analysis #41

Closed
anilhelvaci opened this issue Oct 26, 2022 · 16 comments
Closed

Governance Analysis #41

anilhelvaci opened this issue Oct 26, 2022 · 16 comments
Assignees
Milestone

Comments

@anilhelvaci
Copy link
Owner

anilhelvaci commented Oct 26, 2022

Requirements

The parameters to be controlled by governance are listed in the bounty specification. Seems like they all can be controlled by one ParamManager controlled by a contractHelper.

Current Structure

There's a ParamManager for every pool in the protocol. These ParamManagers are stored in the LendingPool contract. Ideally I want to use the contractHelper.js but not sure how I can make that happen in a scenario where we are required to update the parameters that are directly related to pools.

Steps

  • I interpret the phrase Collateral onboarding controlled by governance as adding a new pool to the protocol should be controlled by governance. And I imagine this is one of the most important functions of the governance. Therefore I'll be starting the implementation with this. Also, as I need to explore and might need to add new components to governance package according to our needs, implementing this step will pave the way for governing other parameters. Check the related issue Adding A New Pool Via Governance Voting #42 for details.
@anilhelvaci anilhelvaci added this to the Governance milestone Oct 26, 2022
@anilhelvaci anilhelvaci self-assigned this Oct 26, 2022
@anilhelvaci
Copy link
Owner Author

anilhelvaci commented Oct 27, 2022

Development Notes

These are notes that I'm leaving to myself as I'm exploring the governace package.

  • getParamMgrRetriever, what does this return?
  • ElectionType.offer_filter needs further explanation
  • When governing an API invocation, how do we see the arguments that will be passed to the API to be invoked?
    • a property called methodArgs is present inside issue object.
  • When opening a question on whether or not mark an asset as collateral, does it make sense to open this question with the ElectionType as an api_invocation or a param_change?
  • Who holds the contractGovernor's creatorFacet?
  • How can a simple voter get to vote on a question?
  • Logic behind electrateTools/getPoserInvitation?
  • Structure of offerConfig for makeVoteInvitation?
  • How do voters get the questionHandle? Is there another way other than using getOpenQuestions?
  • How does the poserInvitation get updated?
    Related Methods
    • getUpdatedPoserFacet
  • What happens if I use paramChangeSpec that does not have a [CONTRACT_ELECTORATE] property?

@anilhelvaci
Copy link
Owner Author

anilhelvaci commented Nov 4, 2022

Initial Design

Below is the initial design I've come to after going over Agoric's existing governance package and Compound Finance's governance method.

I'll refer to the governance token as GOV. This name is arbitrary, might change later.

The general rules around the initial governance design is;

  • Anyone who has at least 1 GOV can vote on proposals.
  • In order for a user to make a proposal, they will need to have some pre-determined amount of GOV tokens.
  • The votes will counted in a weighted manner where the amount of GOV token a voter has is the weight of that voter's vote.

The way I'm planning to implement above behaviors in agoric-sdk governance package is;

Users should lock their GOV tokens inside a ZCFSeat so that we're sure they actually have those tokens.

Electorate


The existing implementation of the an Electorate is the committee.js contract which stores all posed questions in a map that has structure like this;

const allQuestions = MapStore<QuestionHandle, VoteCounterFacets> 

Since users will lock their GOV tokens into our contract we're gonna have to use a ZCFSeat to escrow those tokens. The way I imagine how to integrate a ZCFSeat into the existing structure above is;

/**
@type {{
voteCounterFacet: VoteCounterFacet,
tokenSeat: ZCFSeat
}} QuestionFacet
*/
const allQuestions = MapStore<QuestionHandle, QuestionFacet> 

To achieve desired behavior a collection of methods like below should be implemented;

const verifyCanPoseQuestion = () => {
// Checks if the user has enough GOV tokens to pose a question. This method will be added to the existing `addQuestion` implementation in the committee contract.
};

const vote = (questionHandle) => {
// Enables users to vote on a particular question at a the weight they posses GOV tokens. 
};

const redeem = (questionHandle) => {
// Lets users redeem the GOV tokens they locked for a particular question.
};

const electorateFacet = {
    addQuestion,
    vote,
    redeem,
};

const creatorFacet = {
.
.
.
    getElectorateFacetInvitation: () => getElectorateFacetInvitation(zcf, electorateFacet),  // See ElectorateTools
};

ElectorateTools


This module is gonna be like the existing ElectorateTools but we'll store this in our package and will implement required utility methods for our Electorate here. One obvious method is;

/**
 * @param {ZCF} zcf
 * @param {ElectorateFacet} electorateFacet
 */
const getElectorateFacetInvitation = (zcf, electorateFacet) => {
  const electorateFacetHandler = () => Far(`questionPoser`, electorateFacet);
  return zcf.makeInvitation(electorateFacetHandler, `electorateFacet`);
};

ElectionManager


Work_In_Progress


Below is a very high level overview of the design that aims to show which methods will be added to which component.
lending-protocol-tests-governance-design

anilhelvaci added a commit that referenced this issue Nov 7, 2022
…Work is still in progress, should add unit tests to it.
@Chris-Hibbert
Copy link

getParamMgrRetriever, what does this return?

See README.md

ElectionType.offer_filter needs further explanation

Zoe now has filters that allow a contract or its governance to limit which offers can be exercised. The purpose of this is to allow governance to disable access to the contract in emergencies. My apologies that this hasn't been added to the documentation yet.

Agoric/agoric-sdk@732e229

Who holds the contractGovernor's creatorFacet?

Our bootstrap environment holds those. I suspect this is insufficiently documented. You might ask about it in office hours.

How can a simple voter get to vote on a question?

Their voterFacet can be used to cast ballots on questions they know about. The harder question is how they come to know about votes in progress. The committee's publicFacet has a questionSubscriber that notifies about new questions, and a getOpenQuestions() method that allows one to ask directly. Our voting UI makes use of these.

Logic behind electrateTools/getPoserInvitation?

The creatorFacet in committee.js has a method getPoserInvitation() that allows its caller to get an invitation that can be used to create a new question that the electorate can vote on. When the committee is controlled by an electionManager (e.g. psmCharter.js) that method is tightly held and only a limited set of people can invoke it.

Users should lock their GOV tokens inside a ZCFSeat so that we're sure they actually have those tokens.

I recommend following the attestation model. This allows holders of some tokens to get attestations that they can use to vote and doesn't require locking up the tokens.

@anilhelvaci
Copy link
Owner Author

Thanks @Chris-Hibbert !!!

@anilhelvaci
Copy link
Owner Author

anilhelvaci commented Nov 11, 2022

Question Dump On attestation

2022-11-11

  • How does the attestation module know that the user actually has the assets they're trying to make an attestation for?
    • In a real life scenario, I think the attMaker object lives inside lib-wallet module. So I believe it's safe to assume wallet does some kind of checking before creating the attestation. But I haven't been able to find the related code for this. Keep digging.
  • lienBridge seems like it's connected to the cosmos layer. This is my understanding after reading the README for the stakeFactory package. This might not be the actual case but still struggling to understand what lienBridge is actually good for.

2022-11-14

  • Looks like lienBridge is the actual mechanism that checks if the user actually has the assets they want to create attestation for. I can see from the code that it connects to a lower level layer and queries the state of a particular asset, implicated by its brand, and from that state attMaker decides it can create an attestation for that asset or not. lienBridge depends on a brand object and a bridgeManager object and that bridgeManager depends on a bridgeDevice object in order to connect to lower level layer I mentioned earlier. This whole mechanism is mostly used for stakeFactory so the brand here is BLD. The process of creating the lienBridge of the correct brand, BLD, and accessing the bridgeManager is handled during bootstrap. This makes sense because stakeFactory is part of the inter-protocol. But in our case we need a attMaker and therefore a lienBridge for our own GOV brand, this makes me skeptical about below topics;

    • As a dApp developer do I have access to a bridgeManager or bridgeDevice from a deploy script?
    • If no, how can I create the attMaker and therefore lienBridge objects for my own governance token like here?
    • If above analysis is going towards to wrong direction, what is the correct way for me to create attestations for my own token?
  • Where should we create the attestationFacets: Electorate, ElectionManager or the governedContract(LendingPool)?

Related Code
my-lien.js
attestation.js
bridge.js
attestation/test-userFlow.js

I'd be glad if you could answer above questions 🙏 @Chris-Hibbert @dckc

@dckc
Copy link

dckc commented Nov 14, 2022

  • how can I create the attMaker and therefore lienBridge objects for my own governance token?

The lienBridge makes sense only for tokens where an ERTP brand (such as BLD) represents a cosmos denom (such as ubld). Does the GOV token necessarily have a cosmos denom?

If so, then it would make sense to expand makeStakeReporter to work not just for BLD but for all brands registered with E(bankManager).addAsset(). Then the home.attMaker would work for GOV as well as BLD, provided GOV is registered.

  • As a dApp developer do I have access to a bridgeManager or bridgeDevice from a deploy script?

No; that would allow dApp developers to interfere with chain-wide services.

  • Where should we create the attestationFacets: Electorate, ElectionManager or the governedContract(LendingPool)?

Probably LendingPool. @Chris-Hibbert , does that make sense to you?

Another possibility is a separate contract. BLD Boost was originally a separate contract from the attestation maker, but we folded them together to simplify some things. I can probably dig up the separate contract and you might want to use that.

@Chris-Hibbert
Copy link

Chris-Hibbert commented Nov 14, 2022

Where should we create the attestationFacets: Electorate, ElectionManager or the governedContract(LendingPool)?
Probably LendingPool. @Chris-Hibbert , does that make sense to you?

certainly not the electorate or electionManager. Whatever can validate ownership of the underlying token, which makes some sense for the lendingPool.

Another possibility is a separate contract. BLD Boost was originally a separate contract from the attestation maker, but we folded them together to simplify some things. I can probably dig up the separate contract and you might want to use that.

Yes, that would be helpful. I recall the original version (or at least an earlier version) having tests that started with assets created in the JS layer.

Anil Helvaci => Found it: Agoric/agoric-sdk#3475

@anilhelvaci
Copy link
Owner Author

anilhelvaci commented Nov 14, 2022

The lienBridge makes sense only for tokens where an ERTP brand (such as BLD) represents a cosmos denom (such as ubld). Does the GOV token necessarily have a cosmos denom?

I don't know what it means for an ERTP brand to represent a cosmos denom @dckc. What I had in mind is a simple ERTP fungible token created with zcf.makeZCFMint(). If having a cosmos denom just means that it should have 6 decimal places like BLD then I can definitely do that. But according to:

If so, then it would make sense to expand makeStakeReporter to work not just for BLD but for all brands registered with E(bankManager).addAsset(). Then the home.attMaker would work for GOV as well as BLD, provided GOV is registered.

Requires some changes to the agoric-sdk as well and waiting for this change to come alive probably going to block me for some time.

Another possibility is a separate contract. BLD Boost was originally a separate contract from the attestation maker, but we folded them together to simplify some things. I can probably dig up the separate contract and you might want to use that.

Yes, that would be helpful. I recall the original version (or at least an earlier version) having tests that started with assets created in the JS layer.

This seems like a quicker approach. I would be so glad if you could provide me the separate contract and the tests you mention here.

I want to sum up what I understand just to make sure we're on the same page;

  • There's no way I can use the attestation module as it is right now to mint attestations for fungible assets created with zcf.makeZCFMint or makeIssuerKit. To make it work the brand has to have a cosmos denom and makeStakeReporter should be expanded to work with ERTP assets that have cosmos denom so that home.attMaker can create attestations for those assets.

  • Another possible way is to use a separate contract. The samples shall be provided.

Am I correct? @Chris-Hibbert @dckc ?

I'm still not sure how this separate contract will make sure users actually have the GOV tokens without making them lock their GOV tokens?

@dckc
Copy link

dckc commented Nov 14, 2022

The lienBridge makes sense only for tokens where an ERTP brand (such as BLD) represents a cosmos denom (such as ubld). Does the GOV token necessarily have a cosmos denom?

I don't know what it means for an ERTP brand to represent a cosmos denom @dckc. What I had in mind is a simple ERTP fungible token created with zcf.makeZCFMint().

Oops. I'm afraid the whole discussion of attestations has been a red herring. You did say that you plan for users to put their tokens in escrow with Zoe in order to prove ownership. Attestations are a way to avoid that, since BLD boost is designed to let people participate without transferring their BLD tokens to Zoe.

@anilhelvaci
Copy link
Owner Author

You did say that you plan for users to put their tokens in escrow with Zoe in order to prove ownership. Attestations are a way to avoid that, since BLD boost is designed to let people participate without transferring their BLD tokens to Zoe.

I need to make sure users have the governance token of my protocol before I let them do any governance action. One possible solution I had in mind was to escrow the tokens in Zoe. But this is not mandatory I just need to make sure they have the tokens. You and Chris suggested attestations saying that it is more flexible and users might need their tokens for something else. This makes sense to me. But from what I can understand; attestations, as they are right now, only work for BLD brand and no other ERTP brand. If there's a solution for this, I want to use attestations for governance tokens distributed from lending protocol.

@dckc
Copy link

dckc commented Nov 16, 2022

The attestations used in BLD boost attest that some cosmos-level tokens have a lien against them.

In order to get further in this discussion, I suppose we need to clear this up...

I don't know what it means for an ERTP brand to represent a cosmos denom @dckc.

I'm not sure how to do that briefly. Perhaps in office hours tomorrow?

@anilhelvaci
Copy link
Owner Author

I'm not sure how to do that briefly. Perhaps in office hours tomorrow?

That'd be great, looking forward to it!

anilhelvaci added a commit that referenced this issue Nov 16, 2022
@anilhelvaci
Copy link
Owner Author

Rethink

  • Does Electorate need to be a contract or can it be just a js module? @Chris-Hibbert

@dckc
Copy link

dckc commented Nov 16, 2022

@Chris-Hibbert says it has to be a contract

ref: coreArchitecture.png
(office hours recording to appear)

@anilhelvaci
Copy link
Owner Author

In this session of the office hours it has been clarified that there's no easy way for an ordinary ERTP token can be attested. So moving forward with the escrowing strategy.

anilhelvaci added a commit that referenced this issue Nov 22, 2022
…lectionManager`, implemented base test code for `ElectionManager`, implemented a `dummyGovernedContract.js` to test `ElectionManager`.
anilhelvaci added a commit that referenced this issue Nov 24, 2022
…ed out the required test code for `voteOnQuestion`. Implemented `voteOnQuestion` method body.
anilhelvaci added a commit that referenced this issue Nov 24, 2022
…ue with the assert section of the test.
anilhelvaci added a commit that referenced this issue Nov 28, 2022
@anilhelvaci
Copy link
Owner Author

Governance Implementation Initial Review

Instructions

  • Checkout the branch feature/governance-voting
  • Files subject to review;
    • dapp-pool-lending-protocol/contract/src/governance/*
    • dapp-pool-lending-protocol/contract/test/governance/*

Context

As a result of the conversation above and my own work, I've made the below design

  • src/lendingPoolElectorate.js
    Mostly similar to the committee.js but lets users do weighted voting. Has two additional methods to the poserFacet;

    • voteOnQuestion: Exposed via electorateFacet (replaces poserFacet) to the ElectionManager, to let users vote by escrowing their GOV tokens. The value in the escrowed amount is the share(weight) argument in the submitVote method of the binaryCounter.js.
    • updateTotalSupply: binaryCounter.js needs to know the total number of votes in order to calculate a quorum threshold. In our case the total number of votes is interpreted as total number of shares. We use this method to let the electorate know about the total governance token supply so that it can ask the question with the correct quorum threshold.
  • src/lendingPoolElectionManager.js
    Contains the escrowing logic. Has its own ERTP NFT called POP(Proof of Participation). So that users can redeem their escrowed tokens. Exposes addQuestion and voteOnQuestion methods in its public facet and makes decisions according to the amount of assets escrowed.

    • Constraints
      • The minimum amount of governance tokens required to ask a new question is the 2% of the total supply (Like Compound Protocol).
      • The user asking the question can make their escrowed token to be counted in the voting. They do that via a vote flag in the offerArgs and if it is true we vote with their escrowed weight to the positive position. We assume if one asks a question they wouldn't want to vote Against their own question.
  • test/governanceAssertions.js and test/governanceScenarioHelpers.js
    Contain helper methods for unit tests to avoid code reuse and maintain cleaner tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants