[DONE] Interior mutability for Wasm structs

[cycraig]

Description

Explore replacing newtype wrappers with Rc<RefCell<T>> for interior mutability of struct fields in the Wasm bindings.

Motivation

A common problem we experience in the Wasm bindings is being unable to return mutable references. This leads to us returning clones of fields, often opaquely and unexpectedly to developers.

The way we have addressed this in IotaDocument/WasmDocument, where we need to update sub-fields in its metadata (such as updated and previousMessageId), is to duplicate those fields with a prefix on the Document itself. I.e. have metadataUpdated, metadataPreviousMessageId etc. defined on WasmDocument which internally access and mutate the corresponding fields in the metadata.

We could instead duplicate struct field definitions in their Wasm counterparts and use Rc<RefCell<T>> for interior mutability without cloning data. The problem with this is that Rust interfaces expect the entire struct not just its fields, which would require cloning the data and re-constructing the expected struct.

Example

In Rust, ResolvedIotaDocument contains an IotaDocument with some extra fields.

pub struct ResolvedIotaDocument {
  pub document: IotaDocument,
  pub integration_message_id: MessageId,
  pub diff_message_id: MessageId,
}

In Wasm, the newtype WasmResolvedDocument wraps ResolvedIotaDocument.

#[wasm_bindgen(js_name = ResolvedDocument, inspectable)]
pub struct WasmResolvedDocument(pub(crate) ResolvedIotaDocument);

Accessing the inner document in Wasm requires either cloning it or consuming the WasmResolvedDocument:

  /// Returns the inner DID document.
  ///
  /// NOTE: clones the data. Use `intoDocument()` for efficiency.
  #[wasm_bindgen(getter)]
  pub fn document(&self) -> WasmDocument {
    WasmDocument::from(self.0.document.clone())
  }

  /// Consumes this object and returns the inner DID document.
  ///
  /// NOTE: trying to use the `ResolvedDocument` after calling this will throw an error.
  #[wasm_bindgen(js_name = intoDocument)]
  pub fn into_document(self) -> WasmDocument {
    WasmDocument::from(self.0.document)
  }

Developers will often want to access the inner document and end up writing code that "looks correct" but actually clones the data with each access, and then mutate the copy rather than the original. This leads to serious yet subtle bugs that are difficult to identify without knowing the inner workings of the bindings.

Real (abridged) example of this in the wild:

// Returns a ResolvedDocument (WasmResolvedDocument).
let resolved = await client.resolve(issuerDid);

let newKeys = new KeyPair(KeyType.Ed25519);
let newMethod = VerificationMethod.fromDID(resolved.document.id, newKeys, "example-1");
resolved.document.insertMethod(newMethod, MethodScope.VerificationMethod());

resolved.document.metadataPreviousMessageId = resolved.integrationMessageId;
resolved.document.metadataUpdated = Timestamp.nowUTC();

resolved.document.signSelf(authKey, resolved.document.defaultSigningMethod().id.toString());

const response = await client.publishDocument(resolved.document);

This code runs without errors, but publishes an unchanged version of the document. Why? Because each time resolved.document is called, we return a clone of the inner document. Every mutation in the above example (insertMethod(), signSelf(), setting metadataPreviousMessageId, setting metadataUpdated) therefore operates on an independent copy of the document.

The correct version of this code would be:

let resolved = await client.resolve(issuerDid);
let integrationMessageId = resolved.integrationMessageId;
let document = resolved.intoDocument(); // or resolved.document to avoid consuming resolved

let newKeys = new KeyPair(KeyType.Ed25519);
let newMethod = VerificationMethod.fromDID(document.id, newKeys, "example-1");
document.insertMethod(newMethod, MethodScope.VerificationMethod());

document.metadataPreviousMessageId = integrationMessageId;
document.metadataUpdated = Timestamp.nowUTC();

document.signSelf(authKey, document.defaultSigningMethod().id.toString());

const response = await client.publishDocument(document);

Without knowing this "trick", it is incredibly easy to use and mutate clones of fields unknowingly and get confused when updates do not appear to work.

Suggestion

We could change the definition of WasmResolvedDocument and WasmDocument in the bindings to allow returning "clones" of fields which internally hold a smart pointer to the actual data.

E.g.

#[wasm_bindgen(js_name = Document, inspectable)]
pub struct WasmDocument(pub(crate) Rc<RefCell<IotaDocument>>);

#[wasm_bindgen(js_name = ResolvedDocument, inspectable)]
pub struct WasmResolvedDocument {
	document: WasmDocument,
	integrationMessageId: MessageId,
	diffMessageId: MessageId,
}

We could try go further and apply this pattern to WasmDocument to eliminate the metadataPreviousMessageId etc. workaround. However, we would encounter difficulties since we would lose access to methods on IotaDocument. I.e. any IotaDocument method which takes self/&self/&mut self would be unusuable from WasmDocument since we would have destructured it into its fields, and would need to clone them and reconstruct an IotaDocument for each method call.

We even have this problem with the proposed WasmResolvedDocument since client.resolveDiffHistory() expects a ResolvedIotaDocument. So in the Wasm bindings we need to reconstruct a ResolvedIotaDocument from its fields in WasmResolvedDocument, which involves cloning the document out of the Rc<RefCell> since we cannot safely take ownership of it again (though we do this anyway since we have to clone variables passed to async promises).

This pattern is therefore only suitable for data-only structures that have no or few associated methods.

Open questions

• Is the overhead of Rc<RefCell> worth it?
• Is there any workaround for the problem of needing to reconstruct the original struct when an interface expects it? One possibility would be to change the internal representation of the struct on the Rust side to similarly take Rc<RefCell> when compiled in the bindings so we avoid having to clone its fields, but that seems like an unacceptable amount of work required and performance overhead in the Rust library, if possible at all.
• Is the added maintenance of keeping parity of struct field definitions between Rust and the Wasm bindings worth it?
• Is there a better solution?

[olivereanderson]

I think the overhead of Rc<RefCell> is not really worth it, both in terms of implementation and maintenance (increased risk of memory leaks).

I think a reasonable compromise would be to force all interaction with the fields of the wrapped ResolvedIotaDocument in WasmResolvedDocument to happen through an interface to WasmResolvedDocument, i.e. the getter introduced above:

#[wasm_bindgen(getter)]
  pub fn document(&self) -> WasmDocument;

gets replaced with

#[wasm_bindgen]
pub fn extract_document(&self) -> WasmDocument; 

and we introduce a de-facto setter as well:

#[wasm_bindgen]
pub fn replace_document(&mut self, WasmDocument)

Using this is not 100% idiomatic JS, but it should also not be too annoying.

Alternatively (or in addition to the above possible solution) we could introduce some more methods on WasmResolvedDocument that delegate their responsibilities to IotaDocument. In other words the WasmResolvedDocument would implement methods of the kind

impl WasmResolvedDocument {
    #[wasm_bindgen(js_name = document_insertMethod)]
    pub fn document_insert_method(&mut self, method: IotaVerificationMethod, scope: MethodScope) -> Result<()> ;

note that the delegate crate can help with reducing the amount of boiler plate code needed for this. With this alternative one would then have to replace JS code like resolved.document.insertMethod(newMethod, MethodScope.VerificationMethod()); with resolved.document_insertMethod(newMethod, MethodScope.VerificationMethod()); which unfortunately does not feel so idiomatic even though the only difference is an underscore instead of a dot.

Of course we want our JS bindings to be as ergonomic and natural as possible, but at the same time we have to consider the value of chasing 100 % idiomatic JS against for example spending the time on improving the functionality offered by this library (which of course also benefits JS users).

[PhilippGackstatter]

Renaming the getter would be a nice solution, but if a developer just wants to read a field from the document, they now have to clone the document. That's a worse overhead than using Rc<RefCell>.

Do we really want devs to mutate a document through WasmResolvedDocument? Wouldn't we encourage cloning the document, making modifications and publishing that new version, as in Craig's correct version of the example? I don't see the benefit of duplicating all the methods.

[olivereanderson]

Renaming the getter would be a nice solution, but if a developer just wants to read a field from the document, they now have to clone the document.

This is a very good point. "Renaming the getter" solves one problem, but introduces another. Not sure if there would be a workaround?

I don't see the benefit of duplicating all the methods.

The benefit is that it is more performant as it avoids cloning the document, and it is one way of avoiding the incorrect usage as per Craigs first example. On the other hand this proposed alternative has some other disadvantages (aside from those already mentioned) such as making it harder to learn/read identity examples in another language due to (even more) API differences.

Wouldn't we encourage cloning the document, making modifications and publishing that new version, as in Craig's correct version of the example?

This is yet another alternative I forgot to mention. The "cheapest" option is probably to just encourage correct usage by means of documentation.

[cycraig]

Removing the getters and renaming the functions for clarity is a decent alternative and might work for WasmResolvedDocument (acknowledging the disadvantage raised by @PhilippGackstatter of needing to clone it to read any fields but we have intoDocument() which consumes WasmResolvedDocument as an alternative for efficiency currently).
However, we have the same mutability problem in other places, and this problem is likely to grow as new structs get ported to the Wasm bindings. Other examples are the WasmCredential and WasmPresentation structs, where we cannot sign (add a signature) to those structs without returning a clone currently; Rc<RefCell> would allow in-place signing.

we could introduce some more methods on WasmResolvedDocument that delegate their responsibilities to IotaDocument

This is the approach taken with WasmDocument and WasmDocumentMetadata. While it works there, I agree with @PhilippGackstatter that duplicating the methods seems unnecessary in this specific instance.

This is yet another alternative I forgot to mention. The "cheapest" option is probably to just encourage correct usage by means of documentation.

That's the current approach which isn't really working in my opinion. The fields have doc comments in Typescript but we can't expect people to read the documentation on each and every field when some of them work fine while others have unexpected, silent behaviour.

/**
* Returns the inner DID document.
*
* NOTE: clones the data. Use `intoDocument()` for efficiency.
* @returns {Document}
*/
  readonly document: Document;

[PhilippGackstatter]

I suppose the fundamental limitation is that of JS not being able to encode immutable references. I don't think the overhead of Rc<RefCell> is really that much of a problem, if it fixes usability. One doesn't choose to write programs in JS if performance is that important.

I like the general solution, but as you pointed out, it's difficult to say where we should stop Rc<RefCell>-ing fields. Using pub struct WasmDocument(pub(crate) Rc<RefCell<IotaDocument>>); seems like a good solution, but then the consequent solution would be to also expose WasmDocumentMetadata similarly, which would require making modifications to the Rust crates or clone a lot.

To address the root of the problem, we could implement two versions of each struct: One that is mutable and has all methods and one that is immutable and only has methods that take &self. This way we could return WasmDocument from WasmResolvedDocument::document and return WasmDocumentMut from WasmResolvedDocument::extract_document (or something like that). We could probably write a macro that makes that fairly painless. This way, if someone tried to mutate through WasmDocument they'd get a type error in TypeScript or a panic (one option) in JS. That brings the concept of mutability to JS, at the cost of having two types for each actual type. That would probably solve the root problem, but the cost seems not really worth it and it's not idiomatic. To be clear, I'm not advocating for this solution, I merely thought through it by writing it out.

My preferred solution would be to keep the current version of the bindings. I guess one question is also if this is a frequent problem or an exception. Those who use the bindings as intended won't report problems to us, so we may have a skewed perception?

If we have to make a change I think your suggestion is good. For the specific problem of the WasmDocumentMetadata, I would remove WasmDocument::metadata and copy getters and setters over to WasmDocument, to avoid having to touch the Rust crates.

[cycraig]

I like the general solution, but as you pointed out, it's difficult to say where we should stop Rc<RefCell>-ing fields.

We don't have to (and likely should not) adopt the same solution everywhere. I'm leaning towards Rc<RefCell> for the WasmDocument and WasmResolvedDocument problem, but delegating fields/methods seems like the better option for WasmDocument and WasmDocumentMetadata.

To address the root of the problem, we could implement two versions of each struct: One that is mutable and has all methods and one that is immutable and only has methods that take &self.

Interesting approach. To properly consider it: it also introduces a problem where we are unable to use those structs easily as function parameters across the bindings. For example, when calling client.publish(document) do we take the immutable or mutable version? Say it takes only the immutable version (or vice-versa) and I have a mutable version, do I need to convert it, how? Is it possible to take either version in a parameter using some sort of duck-typed interface in Wasm? This in addition to the points you raised make it appear like too much work to refactor and maintain such an approach.

For the specific problem of the WasmDocumentMetadata, I would remove WasmDocument::metadata and copy getters and setters over to WasmDocument, to avoid having to touch the Rust crates.

I'm leaning towards keeping the delegated getters and setters we have now (metadataUpdated, metadataPreviousMessageId etc.) and just removing the getter annotation from WasmDocument::metadata as a hint that the function call returns a copy and not a reference. We already do not expose setters on WasmDocumentMetadata itself to ensure developers cannot make the mistake of attempting to update a copy of it.

[cycraig]

I guess one question is also if this is a frequent problem or an exception. Those who use the bindings as intended won't report problems to us, so we may have a skewed perception?

According to the npm download statistics of the identity-wasm package, most developers still use version 0.4.2 or below, which means they haven't encountered these problems/bugs yet. I think it's just as likely that someone trying out our library encounters a bug like this and wastes time trying to debug it themselves or drops us as an option entirely without reporting it.

[PhilippGackstatter]

it also introduces a problem where we are unable to use those structs easily as function parameters across the bindings

Thanks for thinking about it. That option is indeed not preferable.

After hearing about more places where this could be used, I'm in favor of adopting this approach carefully, like you said:

We don't have to (and likely should not) adopt the same solution everywhere. I'm leaning towards Rc for the WasmDocument and WasmResolvedDocument problem, but delegating fields/methods seems like the better option for WasmDocument and WasmDocumentMetadata.

[cycraig]

Thank you very much for the considered feedback and alternatives presented.

As an outcome of this discussion I would propose we:

1. Change WasmDocument to hold an Rc<RefCell<IotaDocument>> internally. We should be mindful however that this can cause BorrowError/BorrowMutErrors if used improperly across await boundaries (i.e. this would be the first thing to investigate if we ever encounter such an error).
2. Change WasmResolvedDocument from a newtype wrapper to a struct which holds a WasmDocument along with other equivalent fields in ResolvedIotaDocument.
3. Retain the current delegated setters and getters in WasmDocument for WasmDocumentMetadata.
4. Evaluate other places where similar measures are required to avoid confusion, improve the API, or improve performance by reducing the number of clones. Notably WasmCredential and WasmPresentation for in-place operations with WasmDocument::sign_credential and WasmDocument::sign_presentation may be appropriate, but should not be automatically changed without consideration.
5. Remove getter annotations where a cloned type is returned that could lead to confusion if it has mutable properties, or have relatively significant performance overhead if frequently accessed (and therefore cloned). This would mean leaving it as a function: e.g. document.metadata() rather than document.metadata. We could also consider renaming these functions for clarity but I would prefer to keep API differences to a minimum at the moment and continue noting possibly unexpected behaviour in doc comments.

Overall, this should help reduce silent "accidentally-mutating-a-copy" bugs. I acknowledge the possible performance impact, however I believe the API improvements and reduction in potential user bugs to be worth it. If anyone wants to microbenchmark the effects on runtime and memory in Wasm I would still be interested but as was mentioned, such cases where performance is really important should prefer the Rust library.

Any further comments, alternatives, or disagreements are still very welcome.

Edit: opened PR #607.

[JelleMillenaar]

I acknowledge the possible performance impact, however I believe the API improvements and reduction in potential user bugs to be worth it.

I am with Philipp on this one, you don't choose JS for its performance, otherwise what are you even doing. So, unless I heavily underestimate the performance impact, I am a big fan of maximizing the API quality and not just reducing, but if possible completely removing the "accidentally-mutating-a-copy" bug. This is not really a problem a JS should encounter as they have little to no experience / knowledge to find and understand the bug.

The discussion is hard to follow as a non-Rust dev due to how deep it went into Rust details so I can't give a concrete favor of one solution over another, but personally I am happy to take a performance hit (in JS only) in order to fully fix it. Perhaps @abdulmth or @eike-hass can share their PoV from JS side in terms of how the JS API would work and the importance of performance.

[eike-hass]

I agree with the proposed steps 1-5. I would say step 5 needs to rolled out across the whole API. I actually wasn't aware of the cloning behavior and the implication and I expect that to be a big issue for implementers.
Speaking to the performance overhead, I am super not worried about time complexity since for every meaningful interaction network / node overhead will far outweigh whatever we do. Space complexity on the other hand might be a problem with non trivial data sets, we should not be to quick to disregard that for limited environments like AWS lambdas, google Cloud Functions etc.

[abdulmth]

I completely agree with the outcome of the discussion, better API is worth the performance hit in JavaScript, and to my understanding this performance hit still fine in JavaScript since data gets copied and restructured in the background all the time.

[cycraig]

Update: there are concerns around possible memory leaks with adding more Rc smart pointers. Putting this on hold for now.

Parts of this can still be implemented, however, like removing getter annotations.

See: #607 (comment)