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.

Sign up for GitHub

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

Jump to bottom

Genesis options flag for init #241

Closed
rigelrozanski opened this issue Aug 29, 2017 · 7 comments · Fixed by #243
Closed

Genesis options flag for init #241

rigelrozanski opened this issue Aug 29, 2017 · 7 comments · Fixed by #243
Labels
T: Tests

Comments

@rigelrozanski
Copy link
Contributor

provide a flag --module-options on init to interface with the InitState function which satisfies the Middleware interface. (https://github.com/cosmos/cosmos-sdk/blob/develop/modules/coin/handler.go#L84)

Currently the only way to change genesis options is through manually baked in app options in the genesis file (https://github.com/cosmos/cosmos-sdk/blob/develop/cmd/basecoin/commands/init.go#L117). By adding a flag which has the capability to to initialize a genesis middleware param we eliminate the need for many apps to require their own init.go file with custom genesis creation.

Open to suggestions as to the best format for multiple options to be passed in, so far I was thinking something that might look like:

basecoin init --module-options=<name1>/<optionkey>/<value>,<name2>/<optionkey>/<value>
@rigelrozanski rigelrozanski self-assigned this Aug 29, 2017
@ethanfrey
Copy link
Contributor

I like it.

Some comments:

How about --option as the flag name. The other is very long?

Can we just use the option flag multiple times?

@ethanfrey
Copy link
Contributor

Typically the value will be json and have commas inside making parsing your format difficult.

With the change in the flag itself, I agree with the behavior change.

And maybe we can rename plugin_options in the genesis file as well... But that is probably a different issue

@rigelrozanski
Copy link
Contributor Author

okay --option is good if you don't think it's to ambiguous... Didn't realize we could reuse a flag multiple times... great, this solves the second problem of using comma because of JSON.

@ethanfrey
Copy link
Contributor

I could swear I saw this working with cobra, using: https://godoc.org/github.com/spf13/pflag#StringSlice

But I can't find a proper reference, but someone else did this with stdlib: https://lawlessguy.wordpress.com/2013/07/23/filling-a-slice-using-command-line-flags-in-go-golang/

And some interesting flag handling options here:
https://github.com/spf13/pflag

note also the aliasing and deprecation issues, to help with migrations, and that pesky long_name vs long-name issue

@ethanfrey
Copy link
Contributor

Here is the proof it can be called multiple times: https://github.com/spf13/pflag/blob/master/string_slice.go#L42-L54

@rigelrozanski rigelrozanski mentioned this issue Aug 31, 2017
Merged
@ebuchman
Copy link
Member

relevant #324

@rigelrozanski rigelrozanski mentioned this issue Apr 21, 2018
Closed
@rigelrozanski
Copy link
Contributor Author

resolved in init refactor

alexanderbez pushed a commit that referenced this issue May 26, 2022
@p0mvn
fix: state-sync - app version stored in params (#241)
a68d28e 
Closes: #XXX

## What is the purpose of the change

Backporting and expanding on: #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 #11182
- fixed some issues
- addressed some style comments from code review in #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
sosedoff referenced this issue in figment-networks/cosmos-sdk Jun 4, 2022
@mergify @p0mvn
fix: state-sync - app version stored in params (#241) (#243)
6afaa2e 
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]>
roysc pushed a commit to vulcanize/cosmos-sdk that referenced this issue Jul 16, 2022
@p0mvn @roysc
fix: state-sync - app version stored in multi-store, state sync'd (co…
b62ed67 
…smos#241)

Cherry-pick of:
a68d28e
c3bf09f

osmosis-labs/osmosis#1805
yihuang added a commit to yihuang/cosmos-sdk that referenced this issue Apr 1, 2024
@yihuang
Problem: nested cache store not efficient (cosmos#241)
2484db0 
* Problem: nested cache store not efficient

Solution:
- introduce copy-on-write btree based cache store

temp

* changelog

* rename

* Update store/cachekv/store.go

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
dudong2 pushed a commit to b-harvest/cosmos-sdk that referenced this issue Oct 17, 2024
@yihuang @dudong2
Problem: nested cache store not efficient (cosmos#241)
b7236bc 
* Problem: nested cache store not efficient

Solution:
- introduce copy-on-write btree based cache store

temp

* changelog

* rename

* Update store/cachekv/store.go

Signed-off-by: yihuang <[email protected]>

---------

Signed-off-by: yihuang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
T: Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature/init option cosmos/cosmos-sdk
3 participants
@ebuchman @ethanfrey @rigelrozanski