-
Notifications
You must be signed in to change notification settings - Fork 663
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
TODO's addressing #4972
TODO's addressing #4972
Conversation
clarity/src/vm/clarity_wasm.rs
Outdated
@@ -1530,7 +1518,7 @@ fn pass_argument_to_wasm( | |||
Ok((buffer, offset, adjusted_in_mem_offset)) | |||
} | |||
Value::Sequence(SequenceData::String(CharType::UTF8(_s))) => { | |||
todo!("Value type not yet implemented: {:?}", value) | |||
Err(Error::Wasm(WasmError::UnhandledType)) |
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.
This one should not be an error. This just needs to get implemented. Please leave the todo!
and open an issue for it.
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.
Ok. Reverted.
clarity/src/vm/clarity_wasm.rs
Outdated
Value::Principal(_p) => Err(Error::Wasm(WasmError::UnhandledType)), | ||
Value::CallableContract(_c) => Err(Error::Wasm(WasmError::UnhandledType)), | ||
Value::Tuple(_t) => Err(Error::Wasm(WasmError::UnhandledType)), |
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.
Same for these, but don't worry about opening an issue for these, @Acaccia is already working on them.
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.
Ok. Reverted.
clarity/src/vm/errors.rs
Outdated
@@ -144,6 +144,7 @@ pub enum WasmError { | |||
ExpectedReturnValue, | |||
InvalidIndicator(i32), | |||
Runtime(wasmtime::Error), | |||
UnhandledType, |
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.
This error should instead be InvalidType
. We don't want an error for a type that is not handled yet, just an error for a type that should not be allowed where it was found.
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.
Or you could use the existing InvalidNoTypeInValue
and add InvalidListUnionTypeInValue
.
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.
Done. Added InvalidListUnionTypeInValue
.
clarity/src/vm/errors.rs
Outdated
@@ -182,6 +183,7 @@ impl fmt::Display for WasmError { | |||
write!(f, "Invalid response/optional indicator: {indicator}") | |||
} | |||
WasmError::Runtime(e) => write!(f, "Runtime error: {e}"), | |||
WasmError::UnhandledType => write!(f, "Type not handled"), |
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.
Update this accordingly.
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.
Done
@obycode Made changes to address the errors. Now I understand your intent. Thanks! |
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.
Thanks Chris. Looks good. I think you just need to run the formatter.
cargo +nightly fmt-stacks
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.
👍
No description provided.