-
Notifications
You must be signed in to change notification settings - Fork 144
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
Adding Puppet Actor #627
Adding Puppet Actor #627
Conversation
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.
Honestly, still don't have any idea what this actor is for
vm/actor/src/builtin/puppet/mod.rs
Outdated
rt.validate_immediate_caller_accept_any()?; | ||
|
||
rt.transaction(|st: &mut State, _| { | ||
st.opt_fail = vec![]; |
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.
I don't think this fails in our case (and hope this doesn't ever get hit in our node outside of this, but I don't see why theirs would fail if that type never gets serialized) You might want to write a test or something to check if serializing an empty vec of FailToMarshalCBOR
will error
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.
Just wrote the test and a empty vec of FailToMarshalCBOR can be successfully serialized, atleast the way I have it right now. It only fails if the vector actually has something it.
fn serialize_test() { | ||
let mut v: Vec<FailToMarshalCBOR> = vec![]; | ||
|
||
// Should pass becuase vec is empty | ||
assert!(Serialized::serialize(&v).is_ok()); | ||
|
||
v.push(FailToMarshalCBOR::default()); | ||
|
||
// Should fail becuase vec is no longer empty | ||
assert!(Serialized::serialize(v).is_err()); |
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.
Can you also check the case for runtime_transaction_marshal_cbor_failure
? my intuition is that it won't error as it's expected to
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #566
Other information and links