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

livepeer_cli gas price validation #1602

Closed
j1v opened this issue Aug 13, 2020 · 3 comments · Fixed by #2345
Closed

livepeer_cli gas price validation #1602

j1v opened this issue Aug 13, 2020 · 3 comments · Fixed by #2345

Comments

@j1v
Copy link

j1v commented Aug 13, 2020

Describe the bug
Seems like livepeer_cli accepts a new line as input, leading to unexpected behaviour

To Reproduce
choosing 15. Set Eth gas price leads prompt Enter new gas price in Wei (enter "0" for automatic)> that expects a number.

If for some reason the input is new line, the result is as follows :

What would you like to do? (default = stats)
1. Get node status
2. View protocol parameters
3. List registered orchestrators
4. Invoke "initialize round"
5. Invoke "bond"
6. Invoke "unbond"
7. Invoke "rebond"
8. Invoke "withdraw stake" (LPT)
9. Invoke "withdraw fees" (ETH)
10. Invoke "claim" (for rewards and fees)
11. Invoke "transfer" (LPT)
12. Invoke "reward"
13. Invoke multi-step "become an orchestrator"
14. Set orchestrator config
15. Set Eth gas price
16. Sign a message
17. Vote in a poll
> 15
Current gas price: 100
Enter new gas price in Wei (enter "0" for automatic)>
>

That is some how misleading.

if at that point user decides to choose other option ( 1-17 ), livepeer_cli accepts that value as gas price for 15.

Screenshots
Screenshot 2020-08-13 at 16 06 52

@kparkins
Copy link
Contributor

kparkins commented Mar 24, 2022

This behavior occurs due to the implementation of the function readBigInt()

   func (w *wizard) readBigInt() *big.Int {
	for {
		fmt.Printf("> ")
		text, err := w.in.ReadString('\n')
		if err != nil {
			log.Crit("Failed to read user input", "err", err)
		}
		if text = strings.TrimSpace(text); text == "" {
			continue
		}
		val, err := lpcommon.ParseBigInt(strings.TrimSpace(text))
		if err != nil {
			log.Error("Invalid input, expected big integer", "err", err)
			continue
		}
		return val
	}
  }

While it is clear why this is happening, namely, the function readBigInt loops until a valid value is parsed, it is unclear what a "better" behavior would be. A simple option that may make it more clear to the end user would be to supply a prompt to the function readBigInt so that each subsequent prompt indicates the value entered would still be the new gas price.

Alternatively, we can just report an error. This issue is present on both the minimum and maximum gas price options.

@yondonfu
Copy link
Member

A simple option that may make it more clear to the end user would be to supply a prompt to the function readBigInt so that each subsequent prompt indicates the value entered would still be the new gas price.

I think this makes sense. So, if the user presses enter, then the following would be displayed again:

Enter new gas price in Wei (enter "0" for automatic)>

so that it is clear that the next value entered will be for the gas price option. A flaw that is present for this option and all of the wizard options for that matter is that there is no way to exit out of the prompt to select another option without doing a CTRL+C and restarting livepeer_cli. Since that seems to be a problem with how the wizard is designed as a whole, I'd recommend treating that as a separate problem to be addressed.

@kparkins
Copy link
Contributor

kparkins commented Mar 29, 2022

I think this makes sense. So, if the user presses enter, then the following would be displayed again:
Enter new gas price in Wei (enter "0" for automatic)>

Pretty much. Displaying an error is also a good idea, I think. Here's how it would look:

Current maximum gas price: 1
Enter new maximum gas price in Wei (enter "0" for no maximum gas price) > f
ERROR[03-29|15:51:13.036] Invalid input, expected big integer      err="failed to parse big integer"
Enter new maximum gas price in Wei (enter "0" for no maximum gas price) >    
ERROR[03-29|15:51:17.679] Invalid input, expected big integer      err="failed to parse big integer"
Enter new maximum gas price in Wei (enter "0" for no maximum gas price) > 2

What would you like to do? (default = stats)
 .... 

As for the CTRL+C question, I agree.

Another thing to note, the CLI accepts negative values for these parameters. Is that intentional?

E.g. I can set maxGasPrice to -1, bonding amount, etc.

Also, this behavior affects a bunch of other CLI options (haven't completely listed them all), This fix would affect bonding amount, token transfer, rebond, and gas price settings. There are likely more affected options for parameters that are not set via readBigInt. A quick look at wizard.go would indicate as much. Not sure if this is in scope to be addressed as well or if this is worth another issue that has broader scope.

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