Fixing tx-builder-config-builder to take references and not values

[vsubhuman]

No description provided.

[vsubhuman]

Missed this when we've been merging the config PR itself, but if used with values and not references in JS - the passed values are very much getting consumed as well, which causes way too many hidden problems.

[rooooooooob]

I'm fairly certain this (BigNum passed by-value) is present in other areas. I know it implements Copy in rust but I'm not sure if that is enough to not cause issues when it goes through the wasm-bindgen boundary.

[vsubhuman]

For me it caused problem when being passed by value - that value then gets very counter-intuitively erased when used from JS. I will double-check then where it's being used by-value, might also need to be fixed if this is something that is used from JS as well.

[rooooooooob]

Did you see the comments I left in #261 regarding the config builder?

[ozgrakkurt]

I think wasm-bindgen would handle code breaking because of passing by value. Since that would break how javascript works. For example if you have a function in rust that takes by value fn process_string(s: String) and call it from js like this:

let my_s = "hello world";
process_string(my_s);
console.log(my_s);

It should still print hello world?

There is example here that shows that it works: https://rustwasm.github.io/docs/wasm-bindgen/reference/types/exported-rust-types.html#exported-struct-whatever-rust-types

[rooooooooob]

I think wasm-bindgen would handle code breaking because of passing by value. Since that would break how javascript works.

The problem (at least from what I remember) is when you have non-primitive types that don't have some special into wasm ABI or whatever trait defined where I think it moves it out, since in JS they're just a handle/ptr into wasm linear memory, not an actual JS primitive

So String or u32 is fine but TransactionOutput is not. I'm not 100% sure if it's a problem with BigNum or not, since at least in the context of rust Copy types won't be moved out of, but I don't know how things work around the wasm boundary so it might be worth testing that out.

[vsubhuman]

I think wasm-bindgen would handle code breaking because of passing by value. Since that would break how javascript works. For example if you have a function in rust that takes by value fn process_string(s: String) and call it from js like this:

let my_s = "hello world";
process_string(my_s);
console.log(my_s);

It should still print hello world?

There is example here that shows that it works: https://rustwasm.github.io/docs/wasm-bindgen/reference/types/exported-rust-types.html#exported-struct-whatever-rust-types

The problem happens when we have non-native types, as correctly mentioned by Rob, strings are a very special case because in your example you have a JS type string which is being converted into the Rust type when passed in and the Rust type will probably get consumed in this case, but the original JS type will continue working.

Meanwhile we have a lot of case when a Rust type value is being stored in js and then reused, e.g.

function foo(coinParam: RustModule.BigNum) {
    RustModule.doSomething(coinParam);
    RustModule.doSomethingElse(coinParam);
}

If doSomething will consume the value the coinParam variable in JS will still be pointing to a WASM object, but the object will have null-pointed inside and the second call will fail. This is extremely counter-intuitive from JS perspective and would create a constant need to work around something that is considered as anti-pattern for that language. So we should try to always make sure the Rust is written in such a way that it is as easy usable from JS as possible, when possible.

[ozgrakkurt]

Thank you for the explanation. I think in this case taking self by value can be bad here also. For example if the user creates a config buider in one place and tries to update it conditionally he has to do:

let cfg_builder = CfgBuilder::new();

if (cond) {
    cfg_builder = cfg_builder.pool_deposit(my_deposit); // this is what the user has to do
    cfg_builder.pool_deposit(my_deposit); // this destroys the cfg_builder and makes a bug
}

I am trying to look into this

[vsubhuman]

Did you see the comments I left in #261 regarding the config builder?

@rooooooooob , I saw them now, yes. Left a comment there: #261 (comment)

[vsubhuman]

Thank you for the explanation. I think in this case taking self by value can be bad here also. For example if the user creates a config buider in one place and tries to update it conditionally he has to do:

let cfg_builder = CfgBuilder::new();

if (cond) {
    cfg_builder = cfg_builder.pool_deposit(my_deposit); // this is what the user has to do
    cfg_builder.pool_deposit(my_deposit); // this destroys the cfg_builder and makes a bug
}

I am trying to look into this

Oh, this is a really good point! I will test it out in JS now, If this is really the case - we defo have to change then

[ozgrakkurt]

Thank you for the explanation. I think in this case taking self by value can be bad here also. For example if the user creates a config buider in one place and tries to update it conditionally he has to do:

let cfg_builder = CfgBuilder::new();

if (cond) {
    cfg_builder = cfg_builder.pool_deposit(my_deposit); // this is what the user has to do
    cfg_builder.pool_deposit(my_deposit); // this destroys the cfg_builder and makes a bug
}

I am trying to look into this

Oh, this is a really good point! I will test it out in JS now, If this is really the case - we defo have to change then

I checked and looks like this is the case, so we have to change the api to avoid this

[vsubhuman]

I checked and looks like this is the case, so we have to change the api to avoid this

Yes, double checked, the third line in this example crashes:

const x = this.WalletV4.TransactionBuilderConfigBuilder.new();
x.max_tx_size(42);
x.max_value_size(42);

@ozgrakkurt , can you plz create a separate branch an a PR that fixed this?

[ozgrakkurt]

I think the cleanest solution to this might be:

• we ditch the builder pattern
• we make TransactionBuilderConfig implement serde::Deserialize

this way user can construct this like a js object and pass it to TransactionBuilder::new() like explanined here. So the fields are named, which reduces margin for error. But this means when we add or change a parameter it breaks the api.

Another good thing about this option is we can leave the builder methods as they are so they can be used in Rust. But js users can pass JsValue.

I'm not sure if this is the best way.

Alternatively we can make builder methods take reference of self and return nothing.
So user has to do this:

const x = this.WalletV4.TransactionBuilderConfigBuilder.new();
x.max_tx_size(42);
x.max_value_size(42);

And can't do this:

const x = this.WalletV4.TransactionBuilderConfigBuilder.new()
    .max_tx_size(42)
    .max_value_size(42)
    .build();

@rooooooooob @vsubhuman what are your opinions on this?

[vsubhuman]

this way user can construct this like a js object and pass it to TransactionBuilder::new() like explanined here. So the fields are named, which reduces margin for error. But this means when we add or change a parameter it breaks the api.

Another good thing about this option is we can leave the builder methods as they are so they can be used in Rust. But js users can pass JsValue.

This might be a problem to rely on JSON parsing on the JS-WASM boundary, since we are passing problematic values like BigInt a lot there. The Rust API is designed at the moment is such a way that it forces the JS dev to be secure about stuff like numeric values. If we start introducing hidden parsing from JS objects to Rust - from experience this causes only problems in the long term.

Alternatively we can make builder methods take reference of self and return nothing. So user has to do this:

const x = this.WalletV4.TransactionBuilderConfigBuilder.new();
x.max_tx_size(42);
x.max_value_size(42);

Would it be too big of a problem to clone self on every return? Because then it would be fine with the expectations, e.g.:

const x1 = Builder.new()
  .max_tx_size(...)
  .max_value_size(...)
  ...
  .build();

const b = Builder.new();
b = b.max_tx_size(...);
b = b.max_value_size(...);
...
x2 = b.build();

const b2 = Builder.new()
b2.max_tx_size(...);
b2.max_value_size(...);
...
// b2 did not change and did not crash

This would be 100% compatible with usual builder conventions

[ozgrakkurt]

Yes, now that looks like the best solution to me as well, thanks. I can do that in this branch if it's ok, or I can create a seperate one.

[vsubhuman]

Yes, now that looks like the best solution to me as well, thanks. I can do that in this branch if it's ok, or I can create a seperate one.

Let's create a separate one, meanwhile I'll get this one merged 👌