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

Add store package #1243

Merged
merged 7 commits into from
Aug 23, 2019
Merged

Add store package #1243

merged 7 commits into from
Aug 23, 2019

Conversation

NicolasMahe
Copy link
Member

@NicolasMahe NicolasMahe commented Aug 22, 2019

Add a new store package in database package to create a common implementation of both cosmos kvstore and goleveldb.


TODO:

@krhubert
Copy link
Contributor

no panic handle, for example has method might look like:

func (s *CosmosStore) Has(key []byte) (has bool, err error) {
  defer func() {
    if r := recover(); r != nil {
      var ok bool
      if err, ok = r.(error); !ok {
        err = fmt.Errorf("store: %s", r)
      }
    }
  }()
  has = s.store.Has(key)
  return
}

I know it's ugly, but cosmos store uses panics and there is no other option to handle this ...

@krhubert
Copy link
Contributor

Also do we need store dir under database? I think we can just have database/ or store/, what do you think?

@NicolasMahe
Copy link
Member Author

NicolasMahe commented Aug 22, 2019

no panic handle, for example has method might look like:

func (s *CosmosStore) Has(key []byte) (has bool, err error) {
  defer func() {
    if r := recover(); r != nil {
      var ok bool
      if err, ok = r.(error); !ok {
        err = fmt.Errorf("store: %s", r)
      }
    }
  }()
  has = s.store.Has(key)
  return
}

I know it's ugly, but cosmos store uses panics and there is no other option to handle this ...

Could you create a PR that add the error management?

Also do we need store dir under database? I think we can just have database/ or store/, what do you think?

The files were originally in /store but i moved them in database at it is database related and only use by database. I'm open to move it back. @antho1404 what do you think?

@krhubert
Copy link
Contributor

The files were originally in /store but i moved them in database at it is database related and only use by database. I'm open to move it back. @antho1404 what do you think?

Ok, could be database/store then :)

@antho1404 antho1404 added release:change Pull requests that change something existant and removed release:change Pull requests that change something existant labels Aug 22, 2019
@mesg-bot
Copy link
Member

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/network-implementation-with-cosmossdk/304/15

@antho1404
Copy link
Member

Please fix the comments issue on codacy before merge

database/store/leveldb.go Outdated Show resolved Hide resolved
@NicolasMahe NicolasMahe merged commit 19cd2a9 into dev Aug 23, 2019
@NicolasMahe NicolasMahe deleted the cosmos/store branch August 23, 2019 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants