-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Check attach #225
Check attach #225
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #225 +/- ##
========================================
- Coverage 17.0% 16.8% -0.2%
========================================
Files 37 37
Lines 7379 7476 +97
========================================
Hits 1257 1257
- Misses 6122 6219 +97
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few improvements
}) | ||
}) | ||
}); | ||
|
||
// TODO: check attach ids from data containers are present in operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this line now
let state = item | ||
.as_revealed_state() | ||
.expect("get revealed attached state failed"); | ||
if !self.attachments.keys().any(|&id| id == state.id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !self.attachments.keys().any(|&id| id == state.id) { | |
if !self.attachments.contains_key(&state.id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a question: it's not clear to me why self.contains
, because self
is for the consignment, and no contains
method here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it should be self.attachments.contains_key
`assign.as_attachment()` return the vector(`&[AssignAttach<Seal>`), so I think there should not be a `and_then()` method after that. Then `Attach::as_revealed_state`, there is no `as_reveal_state` in the rgbcore, but `Assign<State, Seal>::as_reveal_state`. Co-authored-by: Dr. Maxim Orlovsky <[email protected]>
I will close this pr, because I have no good idea for |
Description: check attached ids from data containers are present in the consignment.
as_attachment()
I'm not sure if the logic I implemented is correct.