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

docs(adr): ADR-045 Admin module #9708

Closed

Conversation

Mar-Alex
Copy link

Description

This ADR defines an admin module.


Author Checklist

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@github-actions github-actions bot added the T: ADR An issue or PR relating to an architectural decision record label Jul 16, 2021
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @Mar-Alex !

So the functionality laid out here is as far as I understand it is covered by #9438, which will allow the gov module to use x/authz to delegate governance operations to other accounts.

One use case which you describe - changing the balance of a test account - sounds useful but isn't actually covered in your proposed design (because proposal handlers currently can't be arbitrary messages). I do think this an important functionality to consider. i.e. should governance be able to execute any message at all such as changing account balances? There might be different ways to deal with it, but what occurs to me is adding some admin privilege handling in x/authz.

As a general matter of process, I suggest opening an issue or discussion first before submitting an ADR because generally we'll want to talk through various solutions. We do currently have a working group on governance overseeing the work in #9438 as well as governance more broadly. I suggest we continue this discussion in #9438 or a separate issue so the working group can make sure our designs cover all the important use cases.

Copy link
Contributor

@alexanderbez alexanderbez 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 in the context of test or dev networks, this makes a lot of sense. I mostly left linting feedback. But also have two comments:

  1. We should ensure InitGenesis fails for x/admin if there exists no admin(s) in genesis.
  2. Are you planning on copying the proposal types from x/gov or just use them directly? I would make note of that.


## Context

The current proposal workflow is based on a concept of validators (token holders) who vote to accept/reject a proposal. The tallying of those votes is based on the voting power (stake-based voting) of validators and their delegators. This stake-based multi-step approach provides a high level of security necessary for mainnets.
Copy link
Contributor

Choose a reason for hiding this comment

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

note, it's not just validators -- any token holder can vote to accept or reject a proposal.


However, the specifics of testnets - often updates with new features that need to be well tested.

Almost every feature and every scenario require some proposals to be accepted. For example, changing a test account's balance (thanks to blockchain technology) is not as easy as changing some number in a bank database. Any modification of a blockchain state requires a full proposal workflow with the majority of stakeholders to participate in voting. The situation is even more complicated in testnet - there's no motivation for users to participate in voting/governance. The full governance process is too long, so it slows down testing and the delivery of new features.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in the context of testnets mainly, this makes a lot of sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

exactly, this is really only meant for testnet environments (altho technically a chain may want it on mainnet for things like emergency upgrades)

docs/architecture/adr-045-admin-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-045-admin-module.md Outdated Show resolved Hide resolved
docs/architecture/adr-045-admin-module.md Outdated Show resolved Hide resolved
```
type MsgService interface {
// SubmitProposal defines a method to create new proposal with a given content.
SubmitProposal(context.Context, *MsgSubmitProposal) (*MsgSenMsgSubmitProposalResponsedResponse, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SubmitProposal(context.Context, *MsgSubmitProposal) (*MsgSenMsgSubmitProposalResponsedResponse, error)
SubmitProposal(context.Context, *MsgSubmitProposal) (*MsgSubmitProposalResponsedResponse, error)

Was that a copy paste error or intended as before?

Requester string
}

type QueryService interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

could be nice to have some convenience queries like isAdmin

# removing admin
simd tx admin delete-admin [admin_address] --from <mykey>
# query list of admins
simd query admin list
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a command for adding the admin to the genesis file before launch?

@aaronc
Copy link
Member

aaronc commented Jul 19, 2021

Since the SDK is already working on more comprehensive solutions to the problems identified in this ADR, my opinion is that this module doesn't belong in the SDK. Maybe there are cases where this module is useful for testnets and devnets, but it can always live in a standalone repo if there is some immediate need for it before the work in #9438 is finished. Adding a new core module to the SDK is a substantial amount of work involving a lot of QA and testing, and if the use cases are already covered elsewhere, it seems hard to justify adding this.

So with that in mind, I am going to close this PR as a duplicate. Apologies for any inconvenience and I do appreciate the effort here. As I said before, we're happy to have a more thorough discussion of the use cases motivating this work in either #9438 or another issue or discussion which you're welcome to open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants