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

[storage] fix: used the typed_store error type #230

Merged
merged 1 commit into from
Jan 24, 2022
Merged

Conversation

huitseeker
Copy link
Contributor

This demonstrates the use of the following PR:
MystenLabs/mysten-infra#6

Please go and review the mysten-infra PR, once it's merged I'll update dependency pointers here.

This updates the typed_store version to one with a TypedStore error type, rather than using eyre::Error.
As a consequence, it allows a more fine-grained conversion of store errors to FastPay::StorageError.

Copy link
Collaborator

@gdanezis gdanezis left a comment

Choose a reason for hiding this comment

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

Good to go! Many thanks for doing this clean-up after the nice talk you gave on the subject!

@@ -151,7 +151,7 @@ pub enum FastPayError {
#[error("Execution invariant violated")]
ExecutionInvariantViolation,
#[error("Storage error")]
StorageError,
StorageError(#[from] typed_store::rocks::TypedStoreError),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird question, I know: this may lead us to return to the client errors straight from the DB including detailed errors about encoding / decoding. Is there any case where this might leak security sensitive information through this error reporting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question is perfectly valid for the whole FastPayError hierarchy. One of the valid things to do here is to make sure your e.g. crypto signing lib doesn't share TMI, which is one of the things we should add as a TODO in #101 or similar. But there may be more scope of inspection here?

self.objects
.multi_get(_objects)
.map_err(|_| FastPayError::StorageError)
self.objects.multi_get(_objects).map_err(|e| e.into())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there no general Into() from Result<R,F> to Result<R',F'> if there is an R into R' and F into F'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oxade
Copy link
Contributor

oxade commented Jan 21, 2022

Very nice to see this in action
Will try to incorporate in client logic

This updates the typed_store version to one with a TypedStore error type, rather than using `eyre::Error`.
As a consequence, it allows a more fine-grained conversion of store errors to `FastPay::StorageError`.
@huitseeker huitseeker changed the title [storage][Do not merge] fix: used the typed_store error type [storage] fix: used the typed_store error type Jan 24, 2022
@huitseeker huitseeker marked this pull request as ready for review January 24, 2022 13:56
@huitseeker huitseeker merged commit 98cc90a into main Jan 24, 2022
@huitseeker huitseeker deleted the typedstore_errors branch February 2, 2022 13:02
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.

3 participants