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

tg3: Common voting interfaces for tgrade #93

Merged
merged 3 commits into from
Feb 9, 2022
Merged

tg3: Common voting interfaces for tgrade #93

merged 3 commits into from
Feb 9, 2022

Conversation

hashedone
Copy link
Collaborator

Closes #85

@hashedone hashedone marked this pull request as draft February 9, 2022 12:59
@hashedone hashedone marked this pull request as ready for review February 9, 2022 13:48
@hashedone hashedone added this to the 0.6.0 Patchnet Stretch milestone Feb 9, 2022
@hashedone hashedone added breaking Breaking changes (API or State) breaking-api API breaking: Remove, Change (rename, change type), Add non-optional / non-default field breaking-state Storage breaking: Requires migration and removed breaking-state Storage breaking: Requires migration labels Feb 9, 2022
Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

I think it is good. Surprised I didn't seem more changes.
Left a few comments for cleanup, but nothing blocking

@@ -2,7 +2,7 @@ use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use cosmwasm_std::Coin;
use cw3::Vote;
use tg3::Vote;
Copy link
Contributor

Choose a reason for hiding this comment

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

Something seems odd to me here.

If these have different field names, I would expect some diff in this repo when I construct Vote to use points not weight.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of things happens in voting contracts package. In contracts they are just passed around. Thanks to good code abstraction.


Ok(VoterResponse { weight })
Ok(VoterResponse { points })
Copy link
Contributor

Choose a reason for hiding this comment

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

Or is this only used in voting-contract and these two updates (here and below) are the only place weight was exposed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup.

addr: member.addr,
weight: member.points,
})
.map(|Member { addr, points }| VoterDetail { addr, points })
Copy link
Contributor

Choose a reason for hiding this comment

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

nice restructuring


`Vote{proposal_id, vote}` - Given a proposal_id, you can
vote yes, no, abstain or veto. Each signed may have a
different "weight" in the voting and they apply their
Copy link
Contributor

Choose a reason for hiding this comment

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

-> points

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ehh, grepped for READMEs and still miss it :/

Information on who can vote is contract dependent. But
we will work on a common API to display some of this.

`Voter { address }` - returns voting power (weight) of this address, if any
Copy link
Contributor

Choose a reason for hiding this comment

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

points

/// This defines the different ways tallies can happen.
/// Every contract should support a subset of these, ideally all.
///
/// The total_weight used for calculating success as well as the weights of each
Copy link
Contributor

Choose a reason for hiding this comment

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

points

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess I grepped docs and comments for this before moving tg3.

pub votes: Vec<VoteInfo>,
}

/// Returns the vote (opinion as well as weight counted) as well as
Copy link
Contributor

Choose a reason for hiding this comment

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

points (and other usage this file)

/// correct cw4 implementation).
#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
#[serde(rename_all = "snake_case")]
pub enum ThresholdResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this object never constructed in the code?
I cannot find anything making "total_points" that would be needed to construct this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am surprised - I knew it is in ProposalResponse which is returned by voting-contract, but it turns out, that... ProposalResponse is redefined in voting-contract! Without this one. Just will remove along with the whole ProposalResponse.

@hashedone hashedone merged commit 575f6e0 into main Feb 9, 2022
@hashedone hashedone deleted the 85-tg3-voting branch February 9, 2022 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes (API or State) breaking-api API breaking: Remove, Change (rename, change type), Add non-optional / non-default field
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use specilaized tg3 version of voting API for tgrade contracts
2 participants