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

CLI tx commands should generate service Msgs #8306

Closed
4 tasks
amaury1093 opened this issue Jan 12, 2021 · 4 comments · Fixed by #8512
Closed
4 tasks

CLI tx commands should generate service Msgs #8306

amaury1093 opened this issue Jan 12, 2021 · 4 comments · Fixed by #8512
Assignees
Milestone

Comments

@amaury1093
Copy link
Contributor

amaury1093 commented Jan 12, 2021

Summary of Bug

There's currently no way to create transactions that contain ADR-031 service Msgs. At the very least, CLI tx commands should output txs with service Msgs.

x/authz can only exec service Msgs, so is blocked on this.

Version

v0.40.0

Proposal

As discussed internally at Regen, we can outline the following plan:

  • For 0.41, create a temporary struct ServiceMsgClientConn which implements grpc.ClientConn. This struct actually already exists, but is only used for tests. The idea is to:

    • make this struct public, maybe in types/msgservice
    • make all CLI commands use this struct (so that the generated txs use service msgs).
  • For 0.42, try to pull Custom protobuf service code generator #8270 in. Once codegen is in, this temporary ServiceMsgClientConn could be removed, and service Msgs could then be created using TxBuilder (Add ClientConn to TxBuilder to support ADR 031 #7541).

Notes

  • An additional idea would be to add a flag --service-msg (true by default) to generate service Msgs or proto Msgs. This might still allow generating amino Msgs.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093 amaury1093 added this to the v0.41 milestone Jan 12, 2021
@amaury1093 amaury1093 self-assigned this Jan 12, 2021
@amaury1093 amaury1093 changed the title Use a temporary ServiceMsgClientConn to handle service Msgs CLI tx commands should generate service Msgs Jan 12, 2021
@aaronc
Copy link
Member

aaronc commented Jan 12, 2021

I'm not sure why we'd postpone #8270. I did most of the preparatory work already. Delaying it will just mean we have to refactor CLI methods twice right?

@amaury1093
Copy link
Contributor Author

I was under the impression we marked #8270 as 0.42, happy to revisit that in our next grooming call.

Was also hoping to get both x/authz and x/feegrant merged asap, I believe they will get postponed a week or two if we wait for #8270.

@aaronc
Copy link
Member

aaronc commented Jan 12, 2021

Well it's just about tradeoffs. I wouldn't block authz and fee grant getting merged in. But if we're refactoring all tx cli commands we might as well do that once the right way. That refactoring is separate from the basic authz implementation as I understand it.

@amaury1093
Copy link
Contributor Author

That refactoring is separate from the basic authz implementation as I understand it.

IMO the x/authz PR looks good overall, but what feels weird to me before approving are the two following points:

How about for now:

  • put ServiceMsgClientConn somewhere better than simapp/helper (e.g. types/msgservice as proposed)
  • put a TODO somewhere in x/authz to say that it doesn't work
  • and we can close this issue and focus on Custom protobuf service code generator #8270 for 0.41
    ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants