Conversation
DSharifi
left a comment
DSharifi
left a comment
There was a problem hiding this comment.
Looks good, thanks for the change!
Can you just add some unit tests, especially to tcb_info.rs, since some of these conversions now include more logic?
| rtmr1: rtmr1.into(), | ||
| rtmr2: rtmr2.into(), | ||
| rtmr3: rtmr3.into(), | ||
| os_image_hash: os_image_hash.map(|v| v.into()).unwrap_or("".into()), |
There was a problem hiding this comment.
Out of scope for this PR;
I think we should make this field nullable on the wire, I.E. an Option instead of using empty strings.
DSharifi
left a comment
DSharifi
left a comment
There was a problem hiding this comment.
Looks good. Just please split up the unit test that tests 3 cases into separate tests.
| } | ||
|
|
||
| #[test] | ||
| fn TcbInfo__should_succeed_tryfrom_DstackTcbInfo() { |
There was a problem hiding this comment.
I see @netrome's double underscore style is gaining popularity
| #[case("zae1e4", ParsingError::UnexpectedHexCharacter('z', 0))] | ||
| #[case("8ae1e4a", ParsingError::WrongLength(7))] | ||
| #[case("8ae1e4aa", ParsingError::WrongLength(8))] | ||
| fn hexbytes_should_fail_tryfrom_string( |
There was a problem hiding this comment.
some rstest shenanigan here forced me to use snake case, else clippy complains
kevindeforth
left a comment
kevindeforth
left a comment
There was a problem hiding this comment.
Approved, thank you!
But we need to be mindful about contract migration.
I would expect migration to fail if we have old attestations in the contract state, due to this line here:
mpc/crates/contract/src/v3_0_2_state.rs
Line 97 in 8abdba9
Since we don't have any Dstack attestations stored and plan on removing them from the contract altogether, I don't expect this to be a problem.
3 participants
Closes #1740