Skip to content

Conversation

@BowTiedWoo
Copy link
Contributor

This PR adds a trait that is going to be used for writing directly to wasm memory. It is going to be useful for future optimizations where writing to wasm memory will be needed.

@BowTiedWoo BowTiedWoo requested a review from Acaccia February 10, 2025 14:30
@codecov
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.78%. Comparing base (18d3d73) to head (b78a40a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #610      +/-   ##
==========================================
+ Coverage   85.65%   85.78%   +0.13%     
==========================================
  Files          46       46              
  Lines       24554    24700     +146     
  Branches    24554    24700     +146     
==========================================
+ Hits        21032    21190     +158     
+ Misses       1672     1657      -15     
- Partials     1850     1853       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Acaccia Acaccia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add documentation for the trait? We should all start developing this habit.

Comment on lines +1272 to +1282
.set(&mut writer.0, Val::I32(offset))
.map_err(|e| Error::Wasm(WasmError::Runtime(e)))?;

// Call the function
func.call(&mut store, &wasm_args, &mut results)
func.call(&mut writer.0, &wasm_args, &mut results)
.map_err(|e| {
error_mapping::resolve_error(e, instance, &mut store, &epoch, &clarity_version)
error_mapping::resolve_error(e, instance, &mut writer.0, &epoch, &clarity_version)
})?;

// If the function returns a value, translate it into a Clarity `Value`
wasm_to_clarity_value(&return_type, 0, &results, memory, &mut &mut store, epoch)
wasm_to_clarity_value(&return_type, 0, &results, writer.1, &mut writer.0, epoch)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep store and memory, it's clearer.

Comment on lines +135 to 138
let value_type = writer
.0
.data()
.contract_analysis
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super convinced of all those usages of writer.0. The following operations usually have nothing to do with the writer. I understand it's a borrow checker thing, but I'm not super happy with the result...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe destructuring the tuple into caller and memory immediately can avoid repeatedly accessing tuple indices (i.e., writer.0 and writer.1) and help the borrow checker keep track of mutable and immutable references.

let (mut caller, memory) = (caller, memory); on line 127. (haven't tested that)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ BowTiedWoo
❌ Acaccia
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants