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] Add governance functions in Octobay main contract. #7

Closed
mktcode opened this issue Mar 16, 2021 · 4 comments
Closed

[Governance] Add governance functions in Octobay main contract. #7

mktcode opened this issue Mar 16, 2021 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@mktcode
Copy link
Contributor

mktcode commented Mar 16, 2021

@octobay release to @mktcode

Concept: https://octobay.github.io/docs/GOVERNANCE.html
UI Mockups: https://www.figma.com/proto/AzJ6BbBetEwtD6h6p8q1uM/Octobay-Governance-UI?scaling=scale-down

The main Octobay contract needs functions to use the token factory (#5), to launch new governance tokens and the proposal storage (#6), to add new proposals and votes on them.

Creating new tokens and proposals requires oracle checks (OctobayArchive/chainlink-adapters#9, OctobayArchive/chainlink-adapters#10), to make sure only the owner of the respective repository/org is allowed to do that.

When releasing a bounty (either by a user withdrawing funds from a smart bounty or by manual transfer through Octobay's send feature in case of a bounty promise that was never upgraded to a smart bounty but solved anyway) we need to issue governance tokens, based on the USD value of the bounty. As long as we didn't implement ERC20 for bounties, a simple ETH-USD chainlink price feed call will be sufficient. Once ERC20 is implemented, things will become a bit more cumbersome and maybe require a custom chainlink adapter.

@JasperTimm
Copy link
Contributor

@mktcode - hmmm, I can't see any bounty claim implementation in any of the contracts... am I missing something?

@mktcode
Copy link
Contributor Author

mktcode commented Mar 30, 2021

As discussed on Discord, the existing code for this was outdated and removed. New implementation might look something like this:

mapping(bytes32 => string) public issueWithdrawRequests;

function withdrawIssueDeposit(
    address _oracle,
    string calldata _issueId
) public oracleHandlesJob(_oracle, 'claim') returns(bytes32 requestId) {
    (bytes32 jobId, uint256 jobFee) = oracleStorage.getOracleJob(_oracle, 'claim');
    Chainlink.Request memory request =
        buildChainlinkRequest(
            jobId,
            address(this),
            this.confirmWithdrawIssueDeposit.selector
        );
    request.add('githubUserId', userAddressStorage.userIdsByAddress(msg.sender));
    request.add('issueId', _issueId);
    requestId = sendChainlinkRequestTo(_oracle, request, jobFee);
    issueWithdrawRequests[requestId] = _issueId;
}

function confirmWithdrawIssueDeposit(bytes32 _requestId)
    public
    recordChainlinkFulfillment(_requestId)
{
    require(
        issueDepositsAmountByIssueId[issueWithdrawRequests[_requestId]] > 0,
        'This bounty has been withdrawn already.'
    );
    payable(msg.sender).transfer(issueDepositsAmountByIssueId[issueWithdrawRequests[_requestId]]);
    issueDepositsAmountByIssueId[issueWithdrawRequests[_requestId]] = 0;
    // delete issueDeposits[_depositId]; ??? loop through issueDepositIdsByIssueId ???
}

The last line here (delete ...) needs to be discussed. When withdrawing deposits for a user or refunding individual issue deposits, we delete the related deposit entry.

https://github.com/Octobay/contracts/blob/fcfe3efa9b42d2bdf223fd503b40c30eadad40e8/contracts/Octobay.sol#L299

https://github.com/Octobay/contracts/blob/fcfe3efa9b42d2bdf223fd503b40c30eadad40e8/contracts/Octobay.sol#L356

A bounty can actually consist of multiple deposits from multiple users, e.g. a main deposit by the project and then some additional funds from the community. Looping through these deposits (issueDepositIdsByIssueId) might not be a good idea. There could be a situation where a project asks for community funding, resulting in a larger number of individual deposits, potentially making the bounty inaccessible due to exceeding gas limits.

Skipping that and simply setting issueDepositsAmountByIssueId to 0 might be enough here and the deposit entries would still exist but its clear they have been withdrawn.

Alternatively we could restrict a bounty to a single deposit but a little tear would run down my face then. 😢

@mktcode
Copy link
Contributor Author

mktcode commented Mar 31, 2021

For the record:

A problem with the gov token distribution when a bounty is withdrawn is that there's currently no connection between issueId and tokenAddress, so we don't know which token to mint. Letting the oracle call (that is implemented already to check the legitimacy of the withdrawal) return that information is currently not suitable, because the information we need can exceed the current bytes32 limitation for oracle responses. This might be solved by the multiword response feature that Chainlink is working on: https://github.com/smartcontractkit/chainlink/blob/v0.10.3/evm-contracts/src/v0.7/dev/Operator.sol#L199

Another way of solving this could be to work with hashes (md5 -> 32bytes) instead of the actual data.

For now the most reasonable solution is the following:

The maintainer needs to choose which governance token (tokenAddress) to mint for a bounty, when creating it. Even if there is a gov token on the organization level and one specific for the repository, only one of them can be minted. In general we will only mint one specific gov token for a bounty, not multiple ones.

Since everybody can deposit funds on an issue, we need some additional checks in the deposit function.

https://github.com/Octobay/contracts/blob/0b9cef63a44a0f1a40f779cb794d3dc0d6a48add/contracts/Octobay.sol#L343

Could become: depositEthForIssue(string calldata _issueId, address govTokenAddress)

If govTokenAddress == 0 nothing changes, otherwise we need to check whether msg.sender is either the creator of the respective gov token OR the owner of an admin NFT OR an additional oracle check needs to be performed, and only if it passes, govTokenAddress is stored as the token to mint.

Easypeasy... 😅

@mktcode
Copy link
Contributor Author

mktcode commented Apr 16, 2021

for the record: createProposal and castVote are accessible directly through the governor: https://github.com/Octobay/contracts/blob/main/contracts/OctobayGovernor.sol#L103-L177

@mktcode mktcode closed this as completed Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants