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

default output method was changed to 'text' from 'json' #473

Closed
4 tasks
brew0722 opened this issue Mar 22, 2022 · 12 comments · Fixed by #476
Closed
4 tasks

default output method was changed to 'text' from 'json' #473

brew0722 opened this issue Mar 22, 2022 · 12 comments · Fixed by #476

Comments

@brew0722
Copy link
Contributor

brew0722 commented Mar 22, 2022

Summary of Bug

As shown below, in the past, the default output method was json.
-o, --output string Output format (text|json) (default "json")
The cli still tells me that the default is json.

However, the result output is text by default now.

The defaults below appear to have been changed.
https://github.com/line/lbm-sdk/blob/393d1fcd3f3aaf2c632eff74d21d380d70e60a96/client/config/config.go#L15

Version

lbm v0.3.0-rc0
lbm-sdk v0.45.0-rc0

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@brew0722 brew0722 changed the title default output method was changed default output method was changed to 'text' from 'json' Mar 22, 2022
@0Tech
Copy link
Collaborator

0Tech commented Mar 23, 2022

If you don't mind, can you describe your expectation more specifically?

e.g.)
On queries: text
On transactions: json

The example above is the result produced on my local machine.
And I assumed that there is a typo on the version you specified, so I tested it on version 0.43.0-rc0.

@brew0722
Copy link
Contributor Author

brew0722 commented Mar 23, 2022

@0Tech

And I assumed that there is a typo on the version you specified, so I tested it on version 0.43.0-rc0.

0.3.0-rc0 was the lbm version. Sorry to confuse you.
The lbm-sdk version is 0.45.0-rc0 .

On queries: json
On transactions: json

attached an example.
As far as I know, this is common to all response outputs of lbm.

Below is the default output.

user@AL01696733 sample-token % lbm tx wasm store target/wasm32-unknown-unknown/release/sample_token.wasm --from alice --keyring-backend test --chain-id lbm --gas 1000000 -b block -y

code: 0
codespace: ""
data: 0A100A0A73746F72652D636F646512020809
events:

  • attributes:
    • index: true
      key: YWNjX3NlcQ==
      value: bGluazF0d3NmbXVqMjhuZHBoNTRrNG53OGNyd3U4aDljOG1oM3J0eDcwNS84
      type: tx
  • attributes:
    • index: true
      key: c2lnbmF0dXJl
      value: NUpJSmxiUkpCV1FoK3FhcXlCd2Z0Ly9rQjZNS09wNWRXcWZhRFdzTzZuMXN3MWx0eG5TZDlYR2l5OWJRdjBmVk9obFJBOGtSV0hReTA2Q21mT3JZUXc9PQ==
      type: tx
  • attributes:
    • index: true
      key: YWN0aW9u
      value: c3RvcmUtY29kZQ==
      type: message
  • attributes:
    • index: true
      key: bW9kdWxl
      value: d2FzbQ==
    • index: true
      key: c2VuZGVy
      value: bGluazF0d3NmbXVqMjhuZHBoNTRrNG53OGNyd3U4aDljOG1oM3J0eDcwNQ==
      type: message
  • attributes:
    • index: true
      key: Y29kZV9pZA==
      value: OQ==
      type: store_code
      gas_used: "935914"
      gas_wanted: "1000000"
      height: "6717"
      info: ""
      logs:
  • events:
    • attributes:
      • key: action
        value: store-code
      • key: module
        value: wasm
      • key: sender
        value: link1twsfmuj28ndph54k4nw8crwu8h9c8mh3rtx705
        type: message
    • attributes:
      • key: code_id
        value: "9"
        type: store_code
        log: ""
        msg_index: 0
        raw_log: '[{"events":[{"type":"message","attributes":[{"key":"action","value":"store-code"},{"key":"module","value":"wasm"},{"key":"sender","value":"link1twsfmuj28ndph54k4nw8crwu8h9c8mh3rtx705"}]},{"type":"store_code","attributes":[{"key":"code_id","value":"9"}]}]}]'
        timestamp: ""
        tx: null
        txhash: 8D42E9AE0FA3B00D361551B766B42FE35B95CAB15A39A79E0DE7623E493503E6

and below is the json output(using -o json)

user@AL01696733 sample-token % lbm tx wasm store target/wasm32-unknown-unknown/release/sample_token.wasm --from alice --keyring-backend test --chain-id lbm --gas 1000000 -b block -o json -y

{"height":"6737","txhash":"B0FB63BA41F4EA2E69B35662BAAC99D360F3343B07B21AC93B928241DFAE1ABD","codespace":"","code":0,"data":"0A100A0A73746F72652D636F64651202080A","raw_log":"[{"events":[{"type":"message","attributes":[{"key":"action","value":"store-code"},{"key":"module","value":"wasm"},{"key":"sender","value":"link1twsfmuj28ndph54k4nw8crwu8h9c8mh3rtx705"}]},{"type":"store_code","attributes":[{"key":"code_id","value":"10"}]}]}]","logs":[{"msg_index":0,"log":"","events":[{"type":"message","attributes":[{"key":"action","value":"store-code"},{"key":"module","value":"wasm"},{"key":"sender","value":"link1twsfmuj28ndph54k4nw8crwu8h9c8mh3rtx705"}]},{"type":"store_code","attributes":[{"key":"code_id","value":"10"}]}]}],"info":"","gas_wanted":"1000000","gas_used":"935914","tx":null,"timestamp":"","events":[{"type":"tx","attributes":[{"key":"YWNjX3NlcQ==","value":"bGluazF0d3NmbXVqMjhuZHBoNTRrNG53OGNyd3U4aDljOG1oM3J0eDcwNS85","index":true}]},{"type":"tx","attributes":[{"key":"c2lnbmF0dXJl","value":"d2xpYmU3UXZUL0I0d005Y1FCdW80MU54bFhjVXBURVBPa0VZZGQ2OUN1MDlMbSt1Z1ZHako3VzFSb3crSUFYMXcvZUtCUlQramdsVnJUZ2JJME9kaFE9PQ==","index":true}]},{"type":"message","attributes":[{"key":"YWN0aW9u","value":"c3RvcmUtY29kZQ==","index":true}]},{"type":"message","attributes":[{"key":"bW9kdWxl","value":"d2FzbQ==","index":true},{"key":"c2VuZGVy","value":"bGluazF0d3NmbXVqMjhuZHBoNTRrNG53OGNyd3U4aDljOG1oM3J0eDcwNQ==","index":true}]},{"type":"store_code","attributes":[{"key":"Y29kZV9pZA==","value":"MTA=","index":true}]}]}

@0Tech
Copy link
Collaborator

0Tech commented Mar 23, 2022

Confirmed it is reproducible. And the recent cosmos-sdk shows the same behavior.
We'd better report it to cosmos-sdk.

@0Tech
Copy link
Collaborator

0Tech commented Mar 23, 2022

BTW, it seems they prefer yaml format.
cosmos/cosmos-sdk#7978
Still, it's a bug because the cli prints the default output is in json, as you mentioned.

@0Tech
Copy link
Collaborator

0Tech commented Mar 23, 2022

I concluded it is not a bug, let me explain.
cosmos/cosmos-sdk#8953 introduces client config, which has the default output format inside it. And we bumped up recently, so we have the changes too.
The config file overrides default behavior of the client binary, so it's OK to say that the default output format of tx is json.
And because they prefer yaml format, they decided to put "text" to the config file.

Sorry for the confusion.

@0Tech
Copy link
Collaborator

0Tech commented Mar 23, 2022

If you agree with my opinion, feel free to close the issue, or we may continue discussion on the subject.

@brew0722
Copy link
Contributor Author

brew0722 commented Mar 23, 2022

If you agree with my opinion, feel free to close the issue, or we may continue discussion on the subject.

I agree that the 'text' format is more appropriate by default.
It seems that we only need to change the default guide in the cli help below.
-o, --output string Output format (text|json) (default "json")

@0Tech
Copy link
Collaborator

0Tech commented Mar 23, 2022

It seems that we only need to change the default guide in the cli help below.

I think it's OK to print it as is, because the default of the "binary" is always json. Please refer to:

The config file overrides default behavior of the client binary, so it's OK to say that the default output format of tx is json.
And because they prefer yaml format, they decided to put "text" to the config file.

I confirmed that after I removed the corresponding entry in the config file, the client prints the output in json format as expected.
FYI: the path of the client config is $home_dir/config/client.toml

@brew0722
Copy link
Contributor Author

Oh, I understood the meaning above.
This is the result of overwriting by client.toml.

However, I think users may be a little confused.
It's definitely not a bug, but I think it would be more appropriate to match the default output of client.toml and the default output of binary to match the initial behavior.

@0Tech
Copy link
Collaborator

0Tech commented Mar 23, 2022

However, I think users may be a little confused.

I strongly agree on it. If users have no idea about the config file, they may think this is a bug as we did.

@0Tech
Copy link
Collaborator

0Tech commented Mar 24, 2022

How about changing the default value in the config?
If we provide the empty string (""), it would not override the default value of the binary.
I've tested it and confirmed it working as expected.

If you think it's OK to go, I will submit a PR of it.

@brew0722
Copy link
Contributor Author

If we provide the empty string (""), it would not override the default value of the binary.

I agree with your suggestion.
At first I cared about the empty string, but I know that by default the chain-id is also empty generated.
I think it would be good to explain that well in the existing comment in config.toml.

CLI output format (text|json)

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

Successfully merging a pull request may close this issue.

2 participants