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

Add ClientConn to TxBuilder to support ADR 031 #7541

Closed
6 tasks
aaronc opened this issue Oct 14, 2020 · 8 comments
Closed
6 tasks

Add ClientConn to TxBuilder to support ADR 031 #7541

aaronc opened this issue Oct 14, 2020 · 8 comments
Labels
C: Proto Proto definition and proto release

Comments

@aaronc
Copy link
Member

aaronc commented Oct 14, 2020

Summary

This is a follow-up to #7500 to enable creating transactions using the new ServiceMsg approach.

Details

  • Make client.TxBuilder implement grpc.ClientConn. This will allow golang clients to build transactions using the generated MsgClient types. Note that this client methods will need to return nil as the response as the method cannot be executed synchronously Extend ClientConn on TxBuilder #7630
  • Add amino support for ServiceMsg
    • register ServiceMsg concrete type and MsgRequest interface with amino codec
    • register each request msg as a MsgRequest concrete time if an amino codec is optionally provided to MsgServiceRouter
  • add integration tests for at least the x/bank Msg/Send to confirm that transaction submission is working with this method, using both SIGN_MODE_DIRECT and SIGN_MODE_LEGACY_AMINO_JSON
  • create CLI helper function ClientTxConn(client.Context, *pflag.FlagSet), and convert all CLI tx endpoints to use ServiceMsgs.
@aaronc aaronc added this to the v0.40.1 milestone Oct 14, 2020
@aaronc aaronc changed the title ADR 031 TxBuilder ClientConn Add ClientConn to TxBuilder to support ADR 031 Oct 14, 2020
@aaronc
Copy link
Member Author

aaronc commented Oct 21, 2020

@amaurymartiny an alternative (or complentary approach) would be a method which takes a client.Context and returns a ClientConn. This would directly broadcast a tx. Ex:

bankMsgClient := bank.NewMsgClient(ClientTxConn(clientCtx))
// this broadcasts the transaction, return value will depend on broadcast mode
// for block broadcast mode the return value is the actual return value
// for other modes the return value is empty because it's basically asycn
_, err := bankMsgClient.Send(...) 

@amaury1093
Copy link
Contributor

This would directly broadcast a tx.

Nice. I see it as a complentary approach, as we still need to allow txs with multiple msgs. Also not sure who signs the tx in this case, ClientTxConn probably needs at least one more arg.

I'll focus on bankMsgClient.Send(txBuilder) first.

@aaronc
Copy link
Member Author

aaronc commented Oct 22, 2020

This would directly broadcast a tx.

Nice. I see it as a complentary approach, as we still need to allow txs with multiple msgs. Also not sure who signs the tx in this case, ClientTxConn probably needs at least one more arg.

Well we general pull that stuff from the cmd line like this:

			return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)

So maybe ClientTxConn(client.Context, *pflag.FlagSet)

@anilcse anilcse mentioned this issue Dec 18, 2020
9 tasks
@clevinson clevinson added ice-box and removed backlog labels Jan 6, 2021
@aaronc aaronc modified the milestones: v0.40.1, v0.42 Jan 6, 2021
@amaury1093
Copy link
Contributor

Adding ClientConn to TxBuilder might not be the best UX (see #7630 (comment)). I propose to revisit this hand-in-hand with #8270.

@tac0turtle
Copy link
Member

is this still on pause ?

@amaury1093
Copy link
Contributor

amaury1093 commented Jan 17, 2023

IMO this issue shouldn't be started with only this scope in mind, but integrated into the larger client.TxBuilder refactor epic, alongside #8138, #10513 etc

@tac0turtle
Copy link
Member

makes sense wasn't sure if it was still relevant

@tac0turtle tac0turtle added the C: Proto Proto definition and proto release label Mar 6, 2023
@tac0turtle
Copy link
Member

we will be reevaluating all client based things and potentially housing it all in clientv2. Ideally all client based things like this are moved out of the state machine. Secondly most of the client stuff here is used for cli, we shouldnt spend too much time on this as most users are using frontends.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Proto Proto definition and proto release
Projects
None yet
Development

No branches or pull requests

4 participants