Improve metadata handling #1148
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Adjusted, please feel free to re-review it :) |
|
some network issue in last CI: https://github.com/integritee-network/worker/actions/runs/3959520470 Unfortunately I don't have permissions to re-run it |
clangenb
reviewed
Jan 20, 2023
Comment on lines
+40
to
+41
| pub trait NodeMetadataTrait: TeerexCallIndexes + SidechainCallIndexes {} | ||
| impl<T: TeerexCallIndexes + SidechainCallIndexes> NodeMetadataTrait for T {} |
Comment on lines
89
to
117
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::assert_matches::assert_matches; | ||
|
|
||
| #[derive(Default)] | ||
| struct NodeMetadataMock; | ||
|
|
||
| impl NodeMetadataMock { | ||
| fn get_one(&self) -> u32 { | ||
| 1 | ||
| } | ||
| } | ||
| #[test] | ||
| fn get_from_meta_data_returns_error_if_not_set() { | ||
| let repo = NodeMetadataRepository::<NodeMetadataMock>::default(); | ||
|
|
||
| assert_matches!(repo.get_from_metadata(|m| m.get_one()), Err(Error::MetadataNotSet)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn get_from_metadata_works() { | ||
| let repo = NodeMetadataRepository::<NodeMetadataMock>::default(); | ||
| repo.set_metadata(NodeMetadataMock); | ||
|
|
||
| assert_eq!(1, repo.get_from_metadata(|m| m.get_one()).unwrap()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we maybe add this again now?
There was a problem hiding this comment.
Can we maybe add this again now?
Sure, just added it. To do it I had to loosen the trait bound, which I think is OK: it's fine to have the get_from_metadata interface for a wider type of T, but in the real logic we expect MetadataType to be bound by NodeMetadataTrait
clangenb
added
A0-core
|
Description updated! Please feel free to edit it if you prefer some other wordings/want to add extra information :) |
|
Thank you very much, I slightly updated it! |
clangenb
approved these changes
Jan 20, 2023
m-yahya
pushed a commit
to olisystems/BEST-Energy
that referenced
this pull request
Feb 17, 2023
* try to use a more generic trait * fix compile * add more comments * revert to old metadata getter * minor adjustment * make clippy happy * add back the tests and loosen the trait bound
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tries to add a more generic parent trait
NodeMetadataTraitthat is bound by variant call indexes traits. With that, it can simplify the metadata extension by only extendingNodeMetadataTrait, instead of modifying the trait bound across multiple files.This PR also tried to solve #1145 initially. However, the compile type error persists, see the comment in
app-libs/stf/src/trusted_call.rs. It's about the conflicts between standard convert and customizedFromimplementation forStfError. We either have to specify the error type explicitly, or remove the customized implementation and usemap_errdirectly. For now, we'll leave it as it is because it's actually the easiest way to address the compile error.Something similar: https://stackoverflow.com/questions/37347311/how-is-there-a-conflicting-implementation-of-from-when-using-a-generic-type.