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

Start refactoring into storeutils #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

mslipper
Copy link
Contributor

Note that the files in storeutils now return generic error objects rather than sdk.Errors. This is done
in an effort to decouple them from the DEX codebase. To transform the error objects back into sdk.Errors
that are meaningful to the DEX, I've created some wrapper functions in types/errs/errs.go that convert
the error objects into the DEC-appropriate sdk.Error.

storeutils/entity_id.go Outdated Show resolved Hide resolved
storeutils/entity_id.go Outdated Show resolved Hide resolved
storeutils/get_set.go Outdated Show resolved Hide resolved
storeutils/get_set.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress. We need doc strings though

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EntityID could be replaced by sdk.Uint altogether

@mslipper
Copy link
Contributor Author

mslipper commented Dec 31, 2019

Re: EntityID - I'm happy to remove it in favor of sdk.Uint, however I thought I'd bring up the original reasons why we created the EntityID type to make sure that removing it is something we want to do.

The goal of the EntityID type is to disambiguate regular sdk.Uint instances, which are used for things like prices and quantities, from sdk.Uint instances used as primary key equivalents in the store. Having a standalone type disambiguates these two uses at compile time, which in my view helps reduce the potential for errors. It also allows us to define EntityID specific utility methods, which are used by the methods in get_set.go.

Let me know if this isn't enough of a reason to keep the standalone type and I'll remove it.

@mslipper mslipper force-pushed the storeutils-refactor branch from f906c8a to 3293c06 Compare January 7, 2020 23:18
Note that the files in `storeutils` now return generic `error` objects rather than `sdk.Error`s. This is done
in an effort to decouple them from the DEX codebase. To transform the error objects back into `sdk.Error`s
that are meaningful to the DEX, I've created some wrapper functions in `types/errs/errs.go` that convert
the `error` objects into the DEC-appropriate `sdk.Error`.
@mslipper mslipper force-pushed the storeutils-refactor branch from 3293c06 to 5396c82 Compare January 7, 2020 23:34
)

var (
ErrNotFound = errors.New("not found")
Copy link
Contributor

@alessio alessio Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ErrNotFound = errors.New("not found")
ErrorStoreKeyNotFound = errors.New("key not found")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ErrorKeyNotFound would work too


var (
ErrNotFound = errors.New("not found")
ErrAlreadyExists = errors.New("already exists")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ErrAlreadyExists = errors.New("already exists")
ErrorKeyExists = errors.New("key exists")

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of variable renames then this is good to go

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 this pull request may close these issues.

2 participants