Generate a `MsgWirePayForMessage` via CLI #32

evan-forbes · 2021-02-19T17:53:10Z

Description

This PR adds a sub command to the cli to generate a MsgWirePayForMessage transaction.

Usage:
  lazyledgerapp tx lazyledgerapp payForMessage [hexNamespace] [hexMessage] [flags]

Breaking down the above command,

lazyledgerappd is the root command for everything lazyledger-app, including starting the node and generating transactions, but of this can change.
the tx sub-command contains other sub-commands related to generating transactions
the lazyledgerapp sub-command specifies the module that is being used
payForMessage is the sub-command to generate a MsgWirePayForMessage

The only necessary inputs are the namespace and the message, where everything else can be optional with sane defaults.

~~This PR should not be merged until #21 is merged.~~ ✔️

the full --help message:

Usage:
  lazyledgerapp tx lazyledgerapp payForMessage [hexNamespace] [hexMessage] [flags]

Flags:
  -a, --account-number uint      The account number of the signing account (offline mode only)
  -b, --broadcast-mode string    Transaction broadcasting mode (sync|async|block) (default "sync")
      --dry-run                  ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it
      --fees string              Fees to pay along with transaction; eg: 10uatom
      --from string              Name or address of private key with which to sign
      --gas string               gas limit to set per-transaction; set to "auto" to calculate sufficient gas automatically (default 200000)
      --gas-adjustment float     adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored  (default 1)
      --gas-prices string        Gas prices in decimal format to determine the transaction fee (e.g. 0.1uatom)
      --generate-only            Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase is not accessible)
  -h, --help                     help for payForMessage
      --keyring-backend string   Select keyring's backend (os|file|kwallet|pass|test) (default "test")
      --keyring-dir string       The client Keyring directory; if omitted, the default 'home' directory will be used
      --ledger                   Use a connected Ledger device
      --memo string              Memo to send along with transaction
      --node string              <host>:<port> to tendermint rpc interface for this chain (default "tcp://localhost:26657")
      --offline                  Offline mode (does not allow any online functionality
  -s, --sequence uint            The sequence number of the signing account (offline mode only)
      --sign-mode string         Choose sign mode (direct|amino-json), this is an advanced feature
      --timeout-height uint      Set a block timeout height to prevent the tx from being committed past a certain height
  -y, --yes                      Skip tx broadcasting prompt confirmation

Global Flags:
      --chain-id string     The network chain ID
      --home string         directory for config and data (default "/home/evan/.lazyledgerapp")
      --log_format string   The logging format (json|plain) (default "plain")
      --log_level string    The logging level (trace|debug|info|warn|error|fatal|panic) (default "info")
      --trace               print out full stack trace on errors

closes: #31 #35

TODO:

rebase after Implement PreprocessTxs #21 is merged

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 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

codecov-io · 2021-02-19T17:55:52Z

Codecov Report

Merging #32 (db5a91c) into master (1e64b39) will increase coverage by 0.62%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
+ Coverage   15.04%   15.66%   +0.62%     
==========================================
  Files          14       14              
  Lines        1888     1909      +21     
==========================================
+ Hits          284      299      +15     
- Misses       1588     1591       +3     
- Partials       16       19       +3

Impacted Files	Coverage Δ
x/lazyledgerapp/types/tx.pb.go	`0.66% <ø> (ø)`
x/lazyledgerapp/types/payformessage.go	`54.67% <72.72%> (+2.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e64b39...db5a91c. Read the comment docs.

remove extra compile time check

…to is compiled

liamsi · 2021-02-23T16:14:03Z

x/lazyledgerapp/keeper/msg_server.go

+// MsgServer is the server API for Msg service.
+type MsgServer interface {
+	// PayForMessage allows the user to post data to made be available.
+	PayForMessage(context.Context, *types.MsgWirePayForMessage) (*types.MsgPayForMessageResponse, error)
+	// PayForMessage allows the user to post data to made be available.
+	SignedTransactionDataPayForMessage(context.Context, *types.SignedTransactionDataPayForMessage) (*types.SignedTransactionDataPayForMessageResponse, error)
+}
+


What is the relation of this interface to the MsgServer interface in x/lazyledgerapp/types/tx.pb.go?

The MsgServer interface in x/lazyledgerapp/types/tx.pb.go only accounts for rpc endpoints, as the generated code assumes that there will always be 1 enpoint per 1 handler. That is not the case in our situation, where we need 2 handlers, but only 1 endpoint.

What was the reason again to only have one endpoint here?

because each MsgWirePayForMessage will have the information for multiple SignedTransactionDataPayForMessages in it, and one SignedTransactionDataPayForMessage is picked during PreprocessTxs, so having an endpoint for SignedTransactionDataPayForMessage could be confusing.

That makes sense!

liamsi · 2021-02-23T16:14:53Z

x/lazyledgerapp/types/tx.pb.go

-	// PayForMessage allows the user to post data to made be available.
-	SignedTransactionDataPayForMessage(context.Context, *SignedTransactionDataPayForMessage) (*SignedTransactionDataPayForMessageResponse, error)


Why are there changes in this file with no changes in the corresponding proto file?

// Code generated by protoc-gen-gogo. DO NOT EDIT.

I had manually changed them in the last PR when I added the handler for SignedTransactionDataPayForMessage 😬 , which was definitely the wrong approach to change MsgServer without adding an rpc endpoint. It was also my fault for force pushing the repo, otherwise someone would have probably caught it and informed me that it was a bad idea.

does this need to be a separate PR?

No worries! Thanks for the explanation. Hmm, yeah probably a good idea to rip out the rpc/endpoint/handler changes and clean that up in a separate PR.

Actually, if that means self-contained small changes, feel free to address this in this PR. Probably good to open a tiny issue explaining the changes for reference though (amend a closes #XXX referencing that issue too in the opening comment).

liamsi · 2021-02-24T16:36:41Z

x/lazyledgerapp/client/cli/payformessage.go

+// transaction.
+func CmdCreatePayForMessage() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "payForMessage [hexNamespace] [hexMessage]",


Other Tx are in JSON encoding or hex, too? (I mean on the cli)

That is a good question. I'm not sure. In other tx, each argument is parsed individually, and the only ones handling raw bytes are using either amino or the Bech32 address encoding. For example lazyledger-appd tx decode [amino-byte-string] [flags] uses the amino byte string, and all addresses are inputted as a Bech32 string. Should it use amino? Something else to handle bytes?

Should it use amino?

NOOOOOOO! please don't :-D

But seriously, let's go with hex bytes for now. Let's open an issue about the UX/UI of the CLI. I'm also not sure what would be best here. We can address this much later anyway (before mainnet). Probably the main usage won't be over the CLI but from other chains too.

And I find payForMessage [hexNamespace] [hexMessage] clear and simple enough.

liamsi · 2021-02-24T16:46:15Z

x/lazyledgerapp/types/payformessage.go

+func (msg *MsgWirePayForMessage) SignShareCommitments(accName string, ring keyring.Keyring) error {
+	for i, commit := range msg.MessageShareCommitment {
+		bytesToSign, err := msg.GetCommitmentSignBytes(commit.K)
+		if err != nil {
+			return err
+		}
+		sig, _, err := ring.Sign(accName, bytesToSign)
+		if err != nil {
+			return err
+		}
+		msg.MessageShareCommitment[i].Signature = sig
+	}
+	return nil
+}


Dope! Can we create at least a small unit test for this method?

…ternal method for checking/adding padding.

liamsi · 2021-02-24T22:22:42Z

x/lazyledgerapp/types/payformessage.go

-	msg.Message = PadMessage(msg.Message)
+	msg.padMessage()


Shouldn't this be obsolete if the MsgWirePayForMessage was constructed via NewMsgWirePayForMessage?

yes it is redundant, and it reminded me of why I put it there in the first place. The last PR #21 uses MsgWirePayForMessages in the tests but obviously doesn't use the new NewMsgWirePayForMessage function, so those tests need a quick refactor along with getting rid of this msg.padMessage

resolved by de38b72 and e15b95b

…orMessage` function

liamsi

✅ 🎉 👍🏼

tzdybal

Looks good. I have only one tiny comment.

tzdybal · 2021-02-25T08:47:00Z

x/lazyledgerapp/client/cli/payformessage.go

+			// decode the message
+			message, err := hex.DecodeString(args[1])
+			if err != nil {
+				return err


For better UX, maybe you can wrap error and provide info what argument (namespace or message) is wrong?

nice catch! You just saved someone some time 🙂 not all heroes wear capes 😉

resolved db5a91c

…ment could not be decoded

x/lazyledgerapp/client/cli/payformessage.go

Co-authored-by: Ismail Khoffi <[email protected]>

liamsi

Left some nit about error wrapping.
See "Wrapping errors with %w": https://blog.golang.org/go1.13-errors

Co-authored-by: Ismail Khoffi <[email protected]>

liamsi

evan-forbes added the enhancement New feature or request label Feb 19, 2021

evan-forbes requested review from tzdybal and liamsi February 19, 2021 17:53

evan-forbes self-assigned this Feb 19, 2021

evan-forbes changed the title ~~Evan/client pfm~~ Generate a MsgWirePayForMessage via CLI Feb 19, 2021

evan-forbes marked this pull request as draft February 23, 2021 12:10

evan-forbes added 3 commits February 23, 2021 09:17

add functions to easily create and sign MsgWirePayForMessages

a48ec72

remove extra compile time check

Move MsgServer interface to keeper to avoid changing it everytime pro…

83e51ec

…to is compiled

implement sub command to generate a MsgWirePayForMessage from the cli

7adb975

evan-forbes force-pushed the evan/client-PFM branch from 80afc2a to 7adb975 Compare February 23, 2021 15:35

evan-forbes marked this pull request as ready for review February 23, 2021 15:42

liamsi reviewed Feb 23, 2021

View reviewed changes

liamsi reviewed Feb 24, 2021

View reviewed changes

evan-forbes added 3 commits February 24, 2021 12:35

add unit test for SignShareCommitment

39a8b9c

pad message during MsgWirePayForMessage creation and add a cleaner in…

39b7c24

…ternal method for checking/adding padding.

add missing check for error

ae8bf20

This was referenced Feb 24, 2021

Figure out ideal CLI UI #38

Closed

Implement Integration Tests for WirePayForMessage #39

Closed

liamsi reviewed Feb 24, 2021

View reviewed changes

evan-forbes added 2 commits February 24, 2021 17:42

remove redundant padding method

de38b72

remove redundant signing test and refactor to use new `NewMsgWirePayF…

e15b95b

…orMessage` function

liamsi previously approved these changes Feb 25, 2021

View reviewed changes

tzdybal previously approved these changes Feb 25, 2021

View reviewed changes

review feedback: cli: wrap hex decoding errors to indicate which argu…

db5a91c

…ment could not be decoded

evan-forbes dismissed stale reviews from tzdybal and liamsi via db5a91c February 25, 2021 15:04

liamsi reviewed Feb 26, 2021

View reviewed changes

x/lazyledgerapp/client/cli/payformessage.go Outdated Show resolved Hide resolved

liamsi reviewed Feb 26, 2021

View reviewed changes

x/lazyledgerapp/client/cli/payformessage.go Outdated Show resolved Hide resolved

review feedback: use appropriate error formatting for wrapping

285c42b

Co-authored-by: Ismail Khoffi <[email protected]>

liamsi reviewed Feb 26, 2021

View reviewed changes

review feedback: use appropriate error formatting for wrapping

e425744

Co-authored-by: Ismail Khoffi <[email protected]>

liamsi approved these changes Feb 26, 2021

View reviewed changes

evan-forbes merged commit e4b0bc4 into master Feb 26, 2021

evan-forbes deleted the evan/client-PFM branch February 26, 2021 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate a `MsgWirePayForMessage` via CLI #32

Generate a `MsgWirePayForMessage` via CLI #32

evan-forbes commented Feb 19, 2021 •

edited

Loading

codecov-io commented Feb 19, 2021 •

edited

Loading

liamsi Feb 23, 2021

evan-forbes Feb 23, 2021 •

edited

Loading

liamsi Feb 23, 2021

evan-forbes Feb 23, 2021 •

edited

Loading

liamsi Feb 23, 2021

liamsi Feb 23, 2021 •

edited

Loading

evan-forbes Feb 23, 2021

evan-forbes Feb 23, 2021

liamsi Feb 23, 2021

liamsi Feb 23, 2021 •

edited

Loading

liamsi Feb 24, 2021

evan-forbes Feb 24, 2021

liamsi Feb 24, 2021 •

edited

Loading

liamsi Feb 24, 2021

liamsi Feb 24, 2021

evan-forbes Feb 24, 2021

evan-forbes Feb 25, 2021

liamsi left a comment

tzdybal left a comment

tzdybal Feb 25, 2021

evan-forbes Feb 25, 2021

liamsi left a comment

liamsi left a comment •

edited

Loading

		// PayForMessage allows the user to post data to made be available.
		SignedTransactionDataPayForMessage(context.Context, SignedTransactionDataPayForMessage) (SignedTransactionDataPayForMessageResponse, error)

Generate a MsgWirePayForMessage via CLI #32

Generate a MsgWirePayForMessage via CLI #32

Conversation

evan-forbes commented Feb 19, 2021 • edited Loading

Description

TODO:

codecov-io commented Feb 19, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

evan-forbes Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

tzdybal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamsi left a comment

Choose a reason for hiding this comment

liamsi left a comment • edited Loading

Choose a reason for hiding this comment

Generate a `MsgWirePayForMessage` via CLI #32

Generate a `MsgWirePayForMessage` via CLI #32

evan-forbes commented Feb 19, 2021 •

edited

Loading

codecov-io commented Feb 19, 2021 •

edited

Loading

evan-forbes Feb 23, 2021 •

edited

Loading

evan-forbes Feb 23, 2021 •

edited

Loading

liamsi Feb 23, 2021 •

edited

Loading

liamsi Feb 23, 2021 •

edited

Loading

liamsi Feb 24, 2021 •

edited

Loading

liamsi left a comment •

edited

Loading