R4R: Implement a simulate-only CLI flag/field for REST endpoints #2181

alessio · 2018-08-29T11:11:36Z

Add an extra simulate flag support to both CLI tx commands and RESTful endpoints to trigger the simulation of an unsigned transaction. Relevant changes:

Turning --dry-run causes the --gas flag to be ignored. The simulation will return the estimate of the gas required to actually run the transaction.
Adjustment is no longer required. It now defaults to 1.0.
In some test cases accounts retrieved from the state do not come with a PubKey. In such cases, a fake secp256k1 key is generated and gas consumption calculated accordingly.

Closes: #2110

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2018-08-29T11:12:15Z

Codecov Report

Merging #2181 into develop will decrease coverage by 0.09%.
The diff coverage is 26.47%.

@@            Coverage Diff             @@
##           develop    #2181     +/-   ##
==========================================
- Coverage    63.58%   63.48%   -0.1%     
==========================================
  Files          137      137             
  Lines         8419     8444     +25     
==========================================
+ Hits          5353     5361      +8     
- Misses        2699     2717     +18     
+ Partials       367      366      -1

alexanderbez

@alessio I left some initial feedback. Thanks!

alexanderbez · 2018-08-29T13:19:28Z

client/flags.go

@@ -58,6 +59,7 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command {
 		c.Flags().Bool(FlagAsync, false, "broadcast transactions asynchronously")
 		c.Flags().Bool(FlagJson, false, "return output in json format")
 		c.Flags().Bool(FlagPrintResponse, true, "return tx response (only works with async = false)")
+		c.Flags().Bool(FlagDryRun, false, "no action; ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it")


I think we can remove no action; as the description gives a good enough...description 👍

I usually aim to make command line flags docs stupid-proof :)
Removing "no action" is not a big deal though 👍

Oh and btw, I took inspiration from man apt-get:

-s, --simulate, --just-print, --dry-run, --recon, --no-act No action; perform a simulation of events that would occur based on the current system state but do not actually change the system. [snip]

LOL sorry, amended now

client/utils/rest.go

alexanderbez · 2018-08-29T13:26:59Z

client/utils/utils.go

@@ -15,57 +15,35 @@ import (
 // DefaultGasAdjustment is applied to gas estimates to avoid tx
 // execution failures due to state changes that might
 // occur between the tx simulation and the actual run.
-const DefaultGasAdjustment = 1.2
+const DefaultGasAdjustment = 1.0


Is it useful or maybe even potentially dangerous to have this defined in two places? Seems its already part of the CLIContext?

IMHO we should remove the adjustment once and for all. The estimate is accurate and It Just Works™.
@cwgoes @jaekwon any thoughts on this?

It may not just work for staking transactions, where previous txs in the same block would adjust the gas required.

I see. What would you suggest then?

Well the gas adjustment is part of the CLIContext. Why not just set it upstream (where the default can be used)? Is that not possible?

A sane default (i.e. != 0) is required for REST endpoints too.

We can really choose any number and revisit during code freeze time.

alexanderbez · 2018-08-29T13:33:12Z

client/utils/utils.go

-func EnrichCtxWithGas(txCtx authctx.TxContext, cliCtx context.CLIContext, name, passphrase string, msgs []sdk.Msg) (authctx.TxContext, error) {
-	txBytes, err := BuildAndSignTxWithZeroGas(txCtx, name, passphrase, msgs)
+// SimulateMsgs simulates the transaction and returns the gas estimate and the adjusted value.
+func SimulateMsgs(txCtx authctx.TxContext, cliCtx context.CLIContext, name string, msgs []sdk.Msg, gas int64) (int64, int64, error) {


I think it'll help understanding if the return arguments are named.

Agreed, will amend

alexanderbez · 2018-08-29T13:39:43Z

x/auth/ante.go

@@ -68,6 +68,9 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler {
 		sigs := stdTx.GetSignatures()
 		signerAddrs := stdTx.GetSigners()
 		msgs := tx.GetMsgs()
+		if simulate {


Can you add a comment explaining this?

alexanderbez · 2018-08-29T13:51:55Z

x/auth/ante.go

@@ -160,14 +159,14 @@ func processSig(

 	// Check account number.
 	accnum := acc.GetAccountNumber()
-	if accnum != sig.AccountNumber {
+	if !simulate && (accnum != sig.AccountNumber) {


Im thinking in the following functions, things might be easer to comprehend if there was just a single simulate or !simulate condition and group everything in that code block.

x/auth/ante.go

alexanderbez · 2018-08-29T13:53:16Z

x/auth/ante.go

+		return nil, sdk.ErrInternal("setting PubKey on signer's account").Result()
+	}
+
+	// Check sig.


Redundant comment I think.

alexanderbez · 2018-08-29T13:58:38Z

x/auth/client/context/context.go

+
+// BuildWithPubKey builds a single message to be signed and
+// attach the public key associated to the given name.
+func (ctx TxContext) BuildWithPubKey(name string, msgs []sdk.Msg) ([]byte, error) {


We don't plan on signing this do we?

Nope, nonetheless I needed it to build a spec of signature-less transaction

Ohhh, I was referring to the godoc 👍 . It might need to be updated.

alessio · 2018-08-29T16:49:57Z

I've addressed your comments @alexanderbez, thanks!

ValarDragon · 2018-08-29T17:26:03Z

client/utils/utils.go

@@ -15,57 +15,35 @@ import (
 // DefaultGasAdjustment is applied to gas estimates to avoid tx
 // execution failures due to state changes that might
 // occur between the tx simulation and the actual run.
-const DefaultGasAdjustment = 1.2
+const DefaultGasAdjustment = 1.0


It may not just work for staking transactions, where previous txs in the same block would adjust the gas required.

client/utils/rest.go

ValarDragon · 2018-08-29T17:33:30Z

x/auth/ante.go

 		return nil, sdk.ErrInvalidSequence(
 			fmt.Sprintf("Invalid account number. Got %d, expected %d", sig.AccountNumber, accnum)).Result()
 	}

 	// Check and increment sequence number.
 	seq := acc.GetSequence()
-	if seq != sig.Sequence {
+	if !simulate && (seq != sig.Sequence) {


I'm confused, you should still have the correct sequence number when simulating a tx?

When simulating, sig would be empty - line 68, sigs := stdTx.GetSignatures(), sigs would just be a 0-length slice.

Makes sense, can you add a comment here explaining that?

ValarDragon · 2018-08-29T17:46:19Z

Approach so far LGTM

alessio · 2018-08-29T17:56:29Z

ACKd, and committed+pushed.

…

On Wed, Aug 29, 2018 at 6:46 PM, Dev Ojha ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In x/auth/ante.go <#2181 (comment)>: > return nil, sdk.ErrInvalidSequence( fmt.Sprintf("Invalid account number. Got %d, expected %d", sig.AccountNumber, accnum)).Result() } // Check and increment sequence number. seq := acc.GetSequence() - if seq != sig.Sequence { + if !simulate && (seq != sig.Sequence) { Makes sense, can you add a comment here explaining that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN_7EblMba-uCJM32Dn72NvU2YrkjK6ks5uVtNrgaJpZM4WRUym> .

-- Alessio Treglia | [email protected] 0416 0004 A827 6E40 BB98 90FB E8A4 8AE5 311D 765A

alexanderbez

Left some more feedback @alessio -- thanks!

PENDING.md

alexanderbez · 2018-08-30T13:43:30Z

client/flags.go

@@ -58,6 +59,7 @@ func PostCommands(cmds ...*cobra.Command) []*cobra.Command {
 		c.Flags().Bool(FlagAsync, false, "broadcast transactions asynchronously")
 		c.Flags().Bool(FlagJson, false, "return output in json format")
 		c.Flags().Bool(FlagPrintResponse, true, "return tx response (only works with async = false)")
+		c.Flags().Bool(FlagDryRun, false, "no action; ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it")


client/utils/rest.go

alexanderbez · 2018-08-30T13:45:13Z

client/utils/utils.go

@@ -15,57 +15,35 @@ import (
 // DefaultGasAdjustment is applied to gas estimates to avoid tx
 // execution failures due to state changes that might
 // occur between the tx simulation and the actual run.
-const DefaultGasAdjustment = 1.2
+const DefaultGasAdjustment = 1.0


Well the gas adjustment is part of the CLIContext. Why not just set it upstream (where the default can be used)? Is that not possible?

client/utils/utils.go

x/auth/ante.go

alexanderbez · 2018-08-30T13:48:29Z

x/auth/client/context/context.go

+
+// BuildWithPubKey builds a single message to be signed and
+// attach the public key associated to the given name.
+func (ctx TxContext) BuildWithPubKey(name string, msgs []sdk.Msg) ([]byte, error) {


Ohhh, I was referring to the godoc 👍 . It might need to be updated.

x/auth/ante.go

alexanderbez

One small remark on a comment section, but otherwise utACK 👍

cwgoes

Pretty good, just a few suggestions.

client/utils/rest.go

x/auth/ante.go

cwgoes

utACK pending merge conflict resolution

Add a simulate only flag '--dry-run' to both CLI tx commands and RESTful endpoints to trigger the simulation of unsigned transactions. * Turning --dry-run on causes the --gas flag to be ignored. The simulation will return the estimate of the gas required to actually run the transaction. * Adjustment is no longer required. It now defaults to 1.0. * In some test cases accounts retrieved from the state do not come with a PubKey. In such cases, a fake secp256k1 key is generated and gas consumption calculated accordingly. Closes: #2110

@alexanderbez

This is to address @alexanderbez's comments

…uence check

alessio · 2018-08-31T17:23:20Z

@cwgoes ready to go 👍

cwgoes

utACK, thanks @alessio

Let's make sure to test this out by hand prior to the 0.25 release.

alessio added tx C:CLI labels Aug 29, 2018

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners August 29, 2018 11:11

alexanderbez reviewed Aug 29, 2018

View reviewed changes

alessio force-pushed the alessio/2110-dry-run branch from 4f10e64 to 60e3383 Compare August 29, 2018 17:10

ValarDragon reviewed Aug 29, 2018

View reviewed changes

alessio changed the title ~~WIP: Implement a simulate-only CLI flag/field for REST endpoints~~ Implement a simulate-only CLI flag/field for REST endpoints Aug 29, 2018

alessio requested a review from zramsay as a code owner August 30, 2018 04:37

alessio force-pushed the alessio/2110-dry-run branch from 941687e to 9c316e2 Compare August 30, 2018 04:57

alessio removed the wip label Aug 30, 2018

alexanderbez changed the title ~~Implement a simulate-only CLI flag/field for REST endpoints~~ R4R: Implement a simulate-only CLI flag/field for REST endpoints Aug 30, 2018

alexanderbez added the ready-for-review label Aug 30, 2018

alexanderbez reviewed Aug 30, 2018

View reviewed changes

alessio force-pushed the alessio/2110-dry-run branch 2 times, most recently from e460e3e to afe48fc Compare August 31, 2018 07:48

alexanderbez reviewed Aug 31, 2018

View reviewed changes

x/auth/ante.go Outdated Show resolved Hide resolved

alexanderbez approved these changes Aug 31, 2018

View reviewed changes

cwgoes suggested changes Aug 31, 2018

View reviewed changes

client/utils/rest.go Outdated Show resolved Hide resolved

x/auth/ante.go Outdated Show resolved Hide resolved

x/auth/ante.go Outdated Show resolved Hide resolved

cwgoes approved these changes Aug 31, 2018

View reviewed changes

alessio added 2 commits August 31, 2018 19:04

Set GasAdjustment in CLIContext when handling HTTP requests

1370ca6

This is to address @alexanderbez's comments

alessio added 2 commits August 31, 2018 19:04

Don't use pointers when you need them not!

122ed3a

Remove obsolete comments

1ed7656

alessio force-pushed the alessio/2110-dry-run branch from 045d444 to cd8f0b4 Compare August 31, 2018 17:04

No need to create a fake slice of signatures nor skip account no./seq…

d84885c

…uence check

alessio force-pushed the alessio/2110-dry-run branch from cd8f0b4 to d84885c Compare August 31, 2018 17:09

cwgoes approved these changes Aug 31, 2018

View reviewed changes

cwgoes merged commit e1981d4 into develop Aug 31, 2018

cwgoes deleted the alessio/2110-dry-run branch August 31, 2018 20:35

R4R: Implement a simulate-only CLI flag/field for REST endpoints #2181

R4R: Implement a simulate-only CLI flag/field for REST endpoints #2181

Conversation

alessio commented Aug 29, 2018 • edited Loading

codecov bot commented Aug 29, 2018 • edited Loading

Codecov Report

alexanderbez left a 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

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

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

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

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

alessio commented Aug 29, 2018

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

ValarDragon commented Aug 29, 2018

alessio commented Aug 29, 2018 via email

alexanderbez left a 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

alexanderbez left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

alessio commented Aug 31, 2018

cwgoes left a comment

Choose a reason for hiding this comment

alessio commented Aug 29, 2018 •

edited

Loading

codecov bot commented Aug 29, 2018 •

edited

Loading