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: dao based governance NTRN-206 #53

Conversation

quasisamurai
Copy link
Contributor

@quasisamurai quasisamurai commented Sep 13, 2022

Task 1
Task 2

This PR implements a neutron's vision of governance. It uses admin module in bound with smart contract bindings, provided w this pr.

testing:

  • make init
  • from this pr ./test_proposal.sh

@quasisamurai quasisamurai marked this pull request as draft September 13, 2022 07:54
@quasisamurai quasisamurai changed the title feat: alternative goverance feat: alternative governance Sep 13, 2022
@quasisamurai quasisamurai force-pushed the feat/alternative-goverance branch 2 times, most recently from 928d8ef to bd94cd7 Compare September 27, 2022 14:33
@quasisamurai quasisamurai marked this pull request as ready for review October 6, 2022 08:14

import "gogoproto/gogo.proto";

// this line is used by starport scaffolding # genesis/proto/import
Copy link
Collaborator

Choose a reason for hiding this comment

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

pls remove


// Query defines the gRPC querier service.
service Query {
// this line is used by starport scaffolding # 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

all these likes should be removed i think

@@ -17,6 +19,8 @@ type NeutronMsg struct {
RegisterInterchainQuery *RegisterInterchainQuery `json:"register_interchain_query,omitempty"`
UpdateInterchainQuery *UpdateInterchainQuery `json:"update_interchain_query,omitempty"`
RemoveInterchainQuery *RemoveInterchainQuery `json:"remove_interchain_query,omitempty"`
AddAdmin *AddAdmin `json:"add_admin,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

why exactly do we need an add admin message available for the contracts? aren't we going to add a single admin to genesis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a very first method i implemented, so i have not removed AddAdmin exactly to leave a space to discussion - "are we need this one?".

cont, err := proto.Marshal(&prop)
if err != nil {
return nil, sdkerrors.Wrap(err, "failed to marshall incoming SubmitProposal message")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

here and everywhere: please add newlines after error checks

msg := admintypes.MsgSubmitProposal{Proposer: contractAddr.String()}

if submitProposal.Proposals.TextProposal != nil {

Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant newline

func (m *CustomMessenger) addAmin(ctx sdk.Context, contractAddr sdk.AccAddress, addAdmin *bindings.AddAdmin) ([]sdk.Event, [][]byte, error) {
response, err := m.PerformAddAmin(ctx, contractAddr, addAdmin)
if err != nil {
ctx.Logger().Debug("PerformSubmitTx: failed to addAdmin",
Copy link
Collaborator

Choose a reason for hiding this comment

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

PerformSubmitTx is a copy-and-paste

wasmQueryPlugin := NewQueryPlugin(ictxKeeper, icqKeeper)

queryPluginOpt := wasmkeeper.WithQueryPlugins(&wasmkeeper.QueryPlugins{
Custom: CustomQuerier(wasmQueryPlugin),
})
messagePluginOpt := wasmkeeper.WithMessageHandlerDecorator(
CustomMessageDecorator(ictxKeeper, icqKeeper),
CustomMessageDecorator(ictxKeeper, icqKeeper, admKeeper),
Copy link
Collaborator

@zavgorodnii zavgorodnii Oct 7, 2022

Choose a reason for hiding this comment

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

maybe give this variable a full name? adminKeeper? not sure we gain anything from losing a couple of characters. also in other places it has a full name

"result", logMsg,
)

// TODO event?
Copy link
Collaborator

Choose a reason for hiding this comment

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

add the event?

"github.com/spf13/cobra"

"github.com/cosmos/cosmos-sdk/client"
// "github.com/cosmos/cosmos-sdk/client/flags"
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented code

return m, nil
}

//func (m *MsgSubmitProposal) GetContent() govtypes.Content { // TODO m.Content.GetCachedValue() returns nil!
Copy link
Collaborator

Choose a reason for hiding this comment

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

commented code

}

func (am AppModule) DefaultGenesis(jsonCodec codec.JSONCodec) json.RawMessage {
//TODO implement me
Copy link
Collaborator

Choose a reason for hiding this comment

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

implement?

}

func (am AppModule) ValidateGenesis(jsonCodec codec.JSONCodec, config client.TxEncodingConfig, message json.RawMessage) error {
//TODO implement me
Copy link
Collaborator

Choose a reason for hiding this comment

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

implement?

)

// NewQuerier creates a new adminmodule Querier instance
func NewQuerier(keeper Keeper, legacyQuerierCdc *codec.LegacyAmino) sdk.Querier {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't think this is the way to go in sdk 0.45; please check the query implementations in our modules

Copy link
Collaborator

@pr0n00gler pr0n00gler left a comment

Choose a reason for hiding this comment

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

Please add tests for the new msg bindings

Title: submitProposal.Proposals.TextProposal.Title,
Description: submitProposal.Proposals.TextProposal.Description,
}
var parameterChanges []paramChange.ParamChange
Copy link
Collaborator

Choose a reason for hiding this comment

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

parameterChanges := make([]paramChange.ParamChange, 0, len(proposal.Changes))

Description: submitProposal.Proposals.TextProposal.Description,
}
var parameterChanges []paramChange.ParamChange
for _, parameterChange := range proposal.Changes {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about making proposal.Changes in bindings.SubmitProposal as paramChange.ParamChange type?
You don't need this loop then.

Copy link
Contributor

@foxpy foxpy left a comment

Choose a reason for hiding this comment

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

This branch breaks interchain_tx_query integration test:
image

@zavgorodnii
Copy link
Collaborator

NOTE FOR EVERYONE: WE DO NOT NEED TO FIX MODULE FORMATTING, ETC.; WE'LL USE INFORMAL ADMIN MODULE LATER ON.

Copy link
Contributor

@foxpy foxpy left a comment

Choose a reason for hiding this comment

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

Please rebase on upstream audit fixes branch since you lag a lot behind it.

pr0n00gler
pr0n00gler previously approved these changes Oct 28, 2022
Copy link
Collaborator

@pr0n00gler pr0n00gler left a comment

Choose a reason for hiding this comment

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

utACK

@quasisamurai quasisamurai changed the title feat: alternative governance feat: dao based governance Nov 25, 2022
@quasisamurai quasisamurai changed the base branch from main to neutron_audit_oak_19_09_2022_fixes November 28, 2022 10:33
@quasisamurai quasisamurai changed the title feat: dao based governance NTRN-206 feat: dao based governance Nov 28, 2022
@quasisamurai quasisamurai changed the title NTRN-206 feat: dao based governance feat: dao based governance NTRN-206 Nov 28, 2022
@pr0n00gler pr0n00gler deleted the feat/alternative-goverance branch September 18, 2023 13:18
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.

4 participants