Feature/init option #243

rigelrozanski · 2017-08-31T00:07:17Z

Closes #241
The approach was to parse the option flag right into the genesis file as it gets created, the rest of the process train is still the same. Multiple --option flags can be used consecutively and it will work. The one problem that seemed to be a huge headache to resolve is to add more complex options that require JSON (such as the existing value to the option coin/issuer) This has to do with some the annoying ways which glide likes to parse information, I tried to come up with a solution but it seemed like to much effort for what it's worth... so for the time being this these options only accept non-json strings for the option value.

adrianbrink · 2017-08-31T06:48:24Z

cmd/basecoin/commands/init.go

+	var optionsStr string
+	sep := ",\n      "
+	optionsRaw := viper.GetStringSlice(FlagOption)
+	if len(optionsRaw) > 0 {


How about pulling out the logic into its own function so that we can write a unit test against it. This applies to L70 - L85.

Originally, I had thought to unit test the way which you have described, but I kind of figured it should be tested like the rest of our commands as a part of the bash testing... (which has been done checkout init-server.sh). Additionally, the results are more relevant in the context of the genesis file as opposed to just a lone string to add in... Anyways, we can do it, not sure if it makes sense since we have bash test

I agree with Rigel, this cli parsing stuff is covered by the bash tests. Ugly but there is not really a better way.

okay leaving

adrianbrink · 2017-08-31T06:51:32Z

cmd/basecoin/commands/init.go

+	sep := ",\n      "
+	optionsRaw := viper.GetStringSlice(FlagOption)
+	if len(optionsRaw) > 0 {
+		optionsStr = sep


Do we need this assignment?

How about:

sed += strings.Join(option[:], sed)

Maybe we should rename sed then :-)

Are you referring to L75? yes we need that because only if options are set to we want to include a (comma and new line) seperator

ethanfrey

Look good, some minor additions, but the biggest issue is not being able to enter json as you mentioned:

 basecoin init $ADDR --option mod/key/val --option 'coin/dings/{"name": "joe"}'
ERROR: invalid argument "coin/dings/{\"name\": \"joe\"}" for "--option" flag: line 1, column 12: bare " in non-quoted-field

Either we need to fix that, or we need to find a way to parse stuff out, so I could use the role:xyz format and the init function would convert it into a json represention of a basecoin actor (which requires init to understand all the formats), or we change the SetInitState() methods to accept non-json data. (eg. the text represenation of basecoin actor or coins or whatever).

ethanfrey · 2017-09-04T13:41:59Z

cmd/basecoin/commands/init.go

+	var optionsStr string
+	sep := ",\n      "
+	optionsRaw := viper.GetStringSlice(FlagOption)
+	if len(optionsRaw) > 0 {


I agree with Rigel, this cli parsing stuff is covered by the bash tests. Ugly but there is not really a better way.

ethanfrey · 2017-09-04T13:43:19Z

cmd/basecoin/commands/init.go

+			options = append(options, option)
+		}
+	}
+	optionsStr += strings.Join(options[:], sep)


s/+=/=/ since it just defaults to the empty string above and put this into the if statement

Moved to the if statement, replaced += ... with = sep + ...

ethanfrey · 2017-09-04T13:44:01Z

cmd/basecoin/commands/init.go

@@ -64,7 +67,24 @@ func initCmd(cmd *cobra.Command, args []string) error {
 		return errors.New("Address must be 20-bytes in hex")
 	}

-	genesis := GetGenesisJSON(viper.GetString(FlagChainID), userAddr)
+	var options []string


options and sep can be moved into the if statement as we only need them if options flags are provided.

👍 addressed

ethanfrey · 2017-09-04T13:45:27Z

tests/cli/init-server.sh

+CLIENT_EXE=basecli
+SERVER_EXE=basecoin
+
+test01initOption() {


nice test that it writes the values. however, it would be good to make sure it started up and could parse this file.
i guess at least using jq to check the genesis file ensures it is proper json, but still, maybe just start the basecoin server and make sure it doesn't return an error.

ethanfrey · 2017-09-04T14:05:30Z

Looking into the parse error. It comes from this code in pflag:

func readAsCSV(val string) ([]string, error) {
        if val == "" {
                return []string{}, nil
        }
        stringReader := strings.NewReader(val)
        csvReader := csv.NewReader(stringReader)
        return csvReader.Read()
}

Which is based on the csv lib:

https://golang.org/src/encoding/csv/reader.go#L397

Not sure how to fake the csv data properly...

ethanfrey · 2017-09-04T14:13:55Z

Okay, reading through the csv parsing code, I determined that if want quotes inside, then the whole field needs to be in quotes, and you must use two consequtive quotes to denote one quote inside...

basecoin init $ADDR --option mod/key/val --option '"coin/dings/{""name"": ""joe""}"'

produces:

   "plugin_options": [
      "coin/issuer", {"app": "sigs", "addr": "1234567890deadbeefaa1234567890deadbeefaa"},
      "mod/key", "val",
      "coin/dings", "{"name": "joe"}"
    ]

which is almost what we want, even if the cli is a bit ugly. one important issue is we note the json is surrounded by quotes on the last line (false), while there are no quotes on the top line (correct).

Want to take another stab at this @rigelrozanski ?
I don't mind having to deal with quotes like that in the cli if it is documented. But maybe others do. And in any case, the output must be cleaner.

One option is to implement our own flag type that does this better than string slice, look at:

https://github.com/spf13/pflag/blob/master/string_slice.go#L122
https://github.com/spf13/pflag/blob/master/string_slice.go#L10-L63

zramsay · 2017-09-04T14:22:06Z

I don't mind having to deal with quotes like that in the cli if it is documented. But maybe others do.

Yeah, I'm not a fan. Even if documented, there's a cognitive overheard of having to do that parsing...reducing simplicity. Can we use a simple config file instead of multiple --option flags?

rigelrozanski · 2017-09-05T05:35:27Z

@zramsay you can already just modify the genesis config file... This is really just an added bonus for extra app functionality at startup. The main intention was to be able to use the SDK library functionality without needing to write your own genesis specifications if you didn't want too, which I don't haha. BUT yes, ultimately one can also program a custom init function which takes custom app-specific flags which are parsed specifically for your apps requirements. App-options as far as I can see are pretty much always optional anyways on top of that.

ethanfrey

Okay, seems a fair fix.

Can you add a test case in bash for this?
With and without json.

rigelrozanski · 2017-09-06T16:24:31Z

All changes addressed. Let me know if anything else needs updating

ethanfrey

LGTM. Let's see how they work in practice, and maybe revisit this options idea later if we want to change the format. Only experience will tell.

Closes: #XXX ## What is the purpose of the change Backporting and expanding on: cosmos#11182 This is an attempt to fix app version bug related to state-sync and tendermit. More context is in the linked-above PR. Upon backporting all relevant commits from the linked-above PR, it was discovered that some integration and simulation tests were failing. With the original design, it was difficult to find the source of the problem due to storing the protocol version in 2 database locations. Therefore, the original work was significantly redesigned to only keep the protocol version in param store. Additionally, the protocol version was assumed to have already been set. Since we are creating a new entry in the database, it is now possible that the protocol version is non-existent at the start. As a result, mechanisms for detecting a missing protocol version were added: - `baseapp.init(...)` * This is called on every restart. Here, we check if the protocol version is already in db. If not, set it to 0 - `baseapp.InitChain(...)` * This is called once at genesis. Here, we always set protocol version to 0 Then, on every upgrade the version gets incremented by 1 ## Brief Changelog - cherry-picked all relevant commits from cosmos#11182 - fixed some issues - addressed some style comments from code review in cosmos#11182 - redesigned to only store protocol version in params store and removed logic for storing it in the upgrade keeper - added logic for detecting missing protocol version and, if missing, setting it to 0 - unit and integration tested - fixed all tests ## Testing and Verifying This change is a trivial rework / code cleanup without any test coverage. This change is already covered by existing tests, such as *(please describe tests)*. TODO: This change needs to be manually tested ## Documentation and Release Note - Does this pull request introduce a new feature or user-facing behavior changes? yes - Is a relevant changelog entry added to the `Unreleased` section in `CHANGELOG.md`? no - How is the feature or change documented? not applicable (cherry picked from commit a68d28e) Co-authored-by: Roman <[email protected]>

…lease chore: create a v0.46.0 release branch with celestia specific changes

* Support RunAtomic API * add unit test

rigelrozanski requested a review from ethanfrey August 31, 2017 00:12

rigelrozanski mentioned this pull request Aug 31, 2017

Move all binaries to examples #242

Merged

adrianbrink suggested changes Aug 31, 2017

View reviewed changes

ethanfrey reviewed Sep 4, 2017

View reviewed changes

rigelrozanski force-pushed the feature/init-option branch from aa41327 to 9c463a9 Compare September 5, 2017 05:57

ethanfrey reviewed Sep 5, 2017

View reviewed changes

Rigel Rozanski and others added 6 commits September 6, 2017 01:19

added init option flag and tests

bfd4ce9

init options tests

ab8505d

init option changelog

dd68d34

added sep string var properly

eae60ce

init --option can now use json

748a1e9

json testing, addressed init option PR comments

4ac089f

rigelrozanski force-pushed the feature/init-option branch from f90c5ac to 4ac089f Compare September 6, 2017 05:19

ethanfrey approved these changes Sep 8, 2017

View reviewed changes

ethanfrey merged commit 83c460a into develop Sep 8, 2017

ethanfrey deleted the feature/init-option branch September 8, 2017 18:45

rootulp referenced this pull request in rootulp/cosmos-sdk Aug 15, 2022

Merge pull request #243 from celestiaorg/evan/celestia-ize-v0.46.0-re…

c20a3b1

…lease chore: create a v0.46.0 release branch with celestia specific changes

yihuang added a commit to yihuang/cosmos-sdk that referenced this pull request Apr 1, 2024

Problem: no API to use the new CoW branch store (cosmos#243)

7d01116

* Support RunAtomic API * add unit test

dudong2 pushed a commit to b-harvest/cosmos-sdk that referenced this pull request Oct 17, 2024

Problem: no API to use the new CoW branch store (cosmos#243)

d66d300

* Support RunAtomic API * add unit test

dudong2 pushed a commit to b-harvest/cosmos-sdk that referenced this pull request Oct 17, 2024

Problem: no API to use the new CoW branch store (cosmos#243)

ea42a34

* Support RunAtomic API * add unit test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/init option #243

Feature/init option #243

rigelrozanski commented Aug 31, 2017

adrianbrink Aug 31, 2017

rigelrozanski Aug 31, 2017

ethanfrey Sep 4, 2017

rigelrozanski Sep 5, 2017

adrianbrink Aug 31, 2017

rigelrozanski Aug 31, 2017

ethanfrey left a comment

ethanfrey Sep 4, 2017

ethanfrey Sep 4, 2017

rigelrozanski Sep 6, 2017

ethanfrey Sep 4, 2017

rigelrozanski Sep 6, 2017

ethanfrey Sep 4, 2017

rigelrozanski Sep 6, 2017

ethanfrey commented Sep 4, 2017

ethanfrey commented Sep 4, 2017

zramsay commented Sep 4, 2017

rigelrozanski commented Sep 5, 2017

ethanfrey left a comment

rigelrozanski commented Sep 6, 2017

ethanfrey left a comment

Feature/init option #243

Feature/init option #243

Conversation

rigelrozanski commented Aug 31, 2017

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

ethanfrey 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

ethanfrey commented Sep 4, 2017

ethanfrey commented Sep 4, 2017

zramsay commented Sep 4, 2017

rigelrozanski commented Sep 5, 2017

ethanfrey left a comment

Choose a reason for hiding this comment

rigelrozanski commented Sep 6, 2017

ethanfrey left a comment

Choose a reason for hiding this comment