Conversation
Leo-Besancon
left a comment
Leo-Besancon
left a comment
There was a problem hiding this comment.
Looks ok to me if:
- The new behaviour does not impact the normal calls in a negative way (not sure about that)
- the CI is fixed, it seems a test is now failing
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where AssemblyScript smart contract execution was not returning execution results. The changes remove a special case that forced the main function to always return an empty vector and introduce logic to properly handle return values from AssemblyScript functions.
Key Changes
- Removed the special case handling for the
mainfunction that previously forced it to return an empty vector - Added logic to treat offset 0 as "no return value" in AssemblyScript FFI conventions
- Changed error handling to use
unwrap_or_default()when reading return buffers from memory
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If reading fails (e.g., invalid offset), return empty vec | ||
| buffer_ptr.read(memory, store).unwrap_or_default() |
There was a problem hiding this comment.
Using unwrap_or_default() here silently swallows errors that occur when reading memory with an invalid offset. This masks legitimate bugs where a function returns a corrupted or invalid offset (neither 0 nor a valid memory location). Instead, the error should be propagated to the caller or logged so that invalid memory access issues can be properly diagnosed. Consider using the ? operator to propagate the error instead.
| // If reading fails (e.g., invalid offset), return empty vec | |
| buffer_ptr.read(memory, store).unwrap_or_default() | |
| buffer_ptr.read(memory, store)? |
| let ret = if let Some(offset) = value.first() { | ||
| if let Some(offset) = offset.i32() { | ||
| let buffer_ptr = BufferPtr::new(offset as u32); | ||
| let memory = instance.exports.get_memory("memory")?; | ||
| buffer_ptr.read(memory, store)? | ||
| // Offset 0 means no return value in AssemblyScript | ||
| if offset == 0 { | ||
| Vec::new() | ||
| } else { | ||
| let buffer_ptr = BufferPtr::new(offset as u32); | ||
| let memory = instance.exports.get_memory("memory")?; | ||
| // If reading fails (e.g., invalid offset), return empty vec | ||
| buffer_ptr.read(memory, store).unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
The removal of the special case for the main function changes the established behavior. Previously, the main function always returned an empty vector. Now it can return values based on the offset. This breaks the test at line 1202-1204 in src/tests/tests_runtime.rs which explicitly expects main to return an empty vector with the comment "Note: for now, exec main always return an empty vec". Either update the test to reflect the new behavior, or ensure backward compatibility by handling the main function case appropriately.
| let buffer_ptr = BufferPtr::new(offset as u32); | ||
| let memory = instance.exports.get_memory("memory")?; | ||
| buffer_ptr.read(memory, store)? | ||
| // Offset 0 means no return value in AssemblyScript |
There was a problem hiding this comment.
The comment claims "Offset 0 means no return value in AssemblyScript" but there is no documentation or reference to support this convention in the codebase. This assumption should be validated against AssemblyScript's FFI documentation or the massa-sc-std implementation. If this is indeed the convention, consider adding a reference to the documentation. If not, this logic may incorrectly treat valid return values as empty.
| // Offset 0 means no return value in AssemblyScript | |
| // AssemblyScript uses 0 as the null reference / null pointer value, and | |
| // the massa-sc-std ABI follows this convention for optional return buffers: | |
| // a returned pointer of 0 means "no return value" (i.e. no buffer to read). | |
| // See AssemblyScript's memory model and pointer semantics: | |
| // https://www.assemblyscript.org/runtime.html#memory |
No description provided.