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

feat: add proposal vote cli arg #3275

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

rickstaa
Copy link
Member

What does this pull request do? Explain your changes. (required)

This pull request gives orchestrators the ability to vote on active treasury proposal directly from the go-livepeer CLI.

Specific updates (required)

  • Adds a new CLI option people can use to vote on active proposals.
  • Adds client bindings to interact with the LivepeerGovernor contract.
  • Adds a new voteOnProposal handler on the server.
  • Adds new tests.
  • Updates outdated contracts.
    • @victorges, @leszko these other contracts were changed when I ran go generate client.go. Can you check if these really need to be updated or that my setup is incorrect.

How did you test each of these updates (required)

I wrote tests and tested the command line argument with a non-orchestrator wallet since we currently don't have a testnet. @victorges, @leszko if you reviewed this I can test with my actual wallet.

Does this pull request close any open issues?

Checklist:

This commit allows orchestrators to vote on active treasury proposal
directly from the go-livepeer CLI.
This commit ensures that the contract bindings are up to date with the
latest version of the livepeer/protocol repository.
@github-actions github-actions bot added the go Pull requests that update Go code label Nov 25, 2024
server/handlers.go Fixed Show fixed Hide fixed
server/handlers.go Fixed Show fixed Hide fixed
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 4.81622% with 751 lines in your changes missing coverage. Please review.

Project coverage is 31.74288%. Comparing base (1dfe3fd) to head (86714fa).

Files with missing lines Patch % Lines
eth/contracts/livepeerToken.go 0.00000% 402 Missing ⚠️
eth/contracts/bondingManager.go 0.00000% 240 Missing ⚠️
cmd/livepeer_cli/wizard_transcoder.go 0.00000% 54 Missing ⚠️
eth/client.go 0.00000% 20 Missing ⚠️
eth/types/contracts.go 0.00000% 12 Missing ⚠️
eth/stubclient.go 0.00000% 9 Missing ⚠️
eth/contracts/controller.go 0.00000% 2 Missing ⚠️
eth/contracts/livepeerTokenFaucet.go 0.00000% 2 Missing ⚠️
eth/contracts/minter.go 0.00000% 2 Missing ⚠️
eth/contracts/poll.go 0.00000% 2 Missing ⚠️
... and 3 more
Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #3275         +/-   ##
===================================================
- Coverage   33.75539%   31.74288%   -2.01251%     
===================================================
  Files            141         142          +1     
  Lines          37357       39842       +2485     
===================================================
+ Hits           12610       12647         +37     
- Misses         24027       26475       +2448     
  Partials         720         720                 
Files with missing lines Coverage Δ
cmd/livepeer_cli/livepeer_cli.go 0.00000% <ø> (ø)
eth/contracts/LivepeerGovernor.go 0.00000% <ø> (ø)
server/handlers.go 55.31078% <100.00000%> (+1.33995%) ⬆️
server/webserver.go 95.87629% <100.00000%> (+0.04296%) ⬆️
eth/contracts/controller.go 0.00000% <0.00000%> (ø)
eth/contracts/livepeerTokenFaucet.go 0.00000% <0.00000%> (ø)
eth/contracts/minter.go 0.00000% <0.00000%> (ø)
eth/contracts/poll.go 0.00000% <0.00000%> (ø)
eth/contracts/roundsManager.go 0.00000% <0.00000%> (ø)
eth/contracts/serviceRegistry.go 0.00000% <0.00000%> (ø)
... and 7 more

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dfe3fd...86714fa. Read the comment docs.

Files with missing lines Coverage Δ
cmd/livepeer_cli/livepeer_cli.go 0.00000% <ø> (ø)
eth/contracts/LivepeerGovernor.go 0.00000% <ø> (ø)
server/handlers.go 55.31078% <100.00000%> (+1.33995%) ⬆️
server/webserver.go 95.87629% <100.00000%> (+0.04296%) ⬆️
eth/contracts/controller.go 0.00000% <0.00000%> (ø)
eth/contracts/livepeerTokenFaucet.go 0.00000% <0.00000%> (ø)
eth/contracts/minter.go 0.00000% <0.00000%> (ø)
eth/contracts/poll.go 0.00000% <0.00000%> (ø)
eth/contracts/roundsManager.go 0.00000% <0.00000%> (ø)
eth/contracts/serviceRegistry.go 0.00000% <0.00000%> (ø)
... and 7 more

... and 1 file with indirect coverage changes

This commit renames the internal reference to the LivepeerGovernor
contract and improves the proposalVote handler data type parsing
behavoir.
@rickstaa rickstaa force-pushed the add_proposal_vote_cli_arg branch from 98fa9ab to 8dc634f Compare November 25, 2024 15:20
@rickstaa rickstaa changed the title add proposal vote cli arg feat: add proposal vote cli arg Nov 25, 2024
@rickstaa rickstaa requested review from leszko, victorges and thomshutt and removed request for leszko January 2, 2025 12:48
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added one comment. Other than that looks good.

If we don't have testnet, then my suggestion is if @rickstaa, you try it with some real proposal with your local Orchestrator and then merge.

@@ -325,6 +330,23 @@ func (c *client) setContracts(opts *bind.TransactOpts) error {

glog.V(common.SHORT).Infof("LivepeerTokenFaucet: %v", c.faucetAddr.Hex())

livepeerGovernorAddr, err := c.GetContract(crypto.Keccak256Hash([]byte("LivepeerGovernor")))
Copy link
Contributor

Choose a reason for hiding this comment

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

This will break the local on-chain setup with the local chain livepeer/geth-with-livepeer-protocol:confluence. It's not the end of the world, but a regression in the dev process.

Two options here:

  1. Quick => Make this resolution here optional (so don't fail if it's not resolved)
  2. Longer => Update livepeer/geth-with-livepeer-protocol:confluence Docker image to include the Governor contract

I think that actually Point 1 may be a better option because we don't risk introducing any regression anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

@leszko Do you know how that docker image is created? When we implemented the treasury I believe I updated the bootstrap scripts from livepeer/protocol to deploy the governor as well. If the docker image is created by simply running that script on a fresh geth, maybe it will just work ®️

Copy link
Member

Choose a reason for hiding this comment

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

Agreed that making this optional would be an acceptable solution tho. Feel free to just do that to unblock this PR @rickstaa!

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

Neat!

Comment on lines 115 to +116
{desc: "Vote in a poll", invoke: w.vote, orchestrator: true},
{desc: "Vote on a proposal", invoke: w.voteOnProposal, orchestrator: true},
Copy link
Member

Choose a reason for hiding this comment

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

doc nit: WDYT of being more specific to avoid confusion?

Suggested change
{desc: "Vote in a poll", invoke: w.vote, orchestrator: true},
{desc: "Vote on a proposal", invoke: w.voteOnProposal, orchestrator: true},
{desc: "Vote in a governance poll", invoke: w.vote, orchestrator: true},
{desc: "Vote on a treasury proposal", invoke: w.voteOnProposal, orchestrator: true},

confirm = w.readStringYesOrNo()
}

fmt.Printf("Do you want to provide a reason for your vote? (y/n) -")
Copy link
Member

Choose a reason for hiding this comment

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

Would be awesome to display these in the dashboard at some point!

Copy link
Contributor

Choose a reason for hiding this comment

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

Mike will be adding it to his vote tracker. At the moment it is just me and Titan setting the reason, but with this PR we might see a lot more Orchestrators add a reason to their vote.

@@ -325,6 +330,23 @@ func (c *client) setContracts(opts *bind.TransactOpts) error {

glog.V(common.SHORT).Infof("LivepeerTokenFaucet: %v", c.faucetAddr.Hex())

livepeerGovernorAddr, err := c.GetContract(crypto.Keccak256Hash([]byte("LivepeerGovernor")))
Copy link
Member

Choose a reason for hiding this comment

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

Agreed that making this optional would be an acceptable solution tho. Feel free to just do that to unblock this PR @rickstaa!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants