Feature/change transactionbuilder creation

[ozgrakkurt]

No description provided.

[vsubhuman]

@ozgrakkurt ,would that be possible to also move this one parameter prefer_pure_change to the config struct but as otpional? Something like Option<boolean> , so that it can be specified, but if it's not - then .build() for config does not fail?

[ozgrakkurt]

yep, that is possible, doing that now.

[rooooooooob]

It's hard to see what's going on due to the cargo fmt (I'm guessing that's what was done). We never had a policy of running it before because many parts of the library were code-generated and we didn't want to do it to make re-generating parts of it easier. It should at the very least be done in its own commit if we're going to be doing it, if not its own PR to make it easy to see what's actually changing.

[ozgrakkurt]

Oh I see about the rustfmt. We can annotate the generated files with #[rustfmt::skip] to not format them. Also I'll roll this change back and do it without fmt to make it clearer.

[ozgrakkurt]

@vsubhuman Also there are some methods like create_tx_builder_full, create_reallistic_tx_builder. Would these methods need to be moved to the builder struct somehow? We could give default values to the fields in the builder struct similar to what I did for prefer_pure_change so user can have this functionality without these methods.

[vsubhuman]

@vsubhuman Also there are some methods like create_tx_builder_full, create_reallistic_tx_builder. Would these methods need to be moved to the builder struct somehow? We could give default values to the fields in the builder struct similar to what I did for prefer_pure_change so user can have this functionality without these methods.

@ozgrakkurt , no no, these are purely for tests. No need to change them

[ozgrakkurt]

I think this is ready to merge if it's ok with you.

…

On Sat, Nov 20, 2021, 09:58 Ruslan Dudin ***@***.***> wrote: @vsubhuman <https://github.com/vsubhuman> Also there are some methods like create_tx_builder_full, create_reallistic_tx_builder. Would these methods need to be moved to the builder struct somehow? We could give default values to the fields in the builder struct similar to what I did for prefer_pure_change so user can have this functionality without these methods. @ozgrakkurt <https://github.com/ozgrakkurt> , no no, these are purely for tests. No need to change them — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#261 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AV37FAZRSIHXGJ6GBOO2QS3UM5BINANCNFSM5IKDBIMA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[ozgrakkurt]

I think self.clone() might be bad here taking self by value and just returning self again might be better

[vsubhuman]

I think self.clone() might be bad here taking self by value and just returning self again might be better

Yup, looks good!

[rooooooooob]

@ozgrakkurt @vsubhuman

It's a few hours too late since it just got merged earlier today, but here are my comments on the API change that might be worth thinking about.

[rooooooooob]

Is this a tradeoff we want to make? By doing this we gain descriptive setters instead of a constructor with unnamed parameters, but we lose the static checks and instead have forgetting one be a runtime error. Most editors will show you the names while typing the code so it would help more for reading code than writing it, but all the required ones are fixed (at a given time) protocol parameters anyway.

This is as opposed to having the required fields be in the constructor and only have setters for optional ones.

If you did that instead then you could also combine these two structs into one and not need a builder at all.

[ozgrakkurt]

A lot of libraries do this pattern in rust, clippy even gives warnings when you have too many arguments in functions. Having a seperate config type is fine I think, since we put the config struct inside the bigger struct instead of moving fields one by one. Also many people don't use editors so having too many arguments makes it more dangerous if a lot of the arguments have the same types and It causes the API to break more frequently because every time you change a argument the function API changes.

[vsubhuman]

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

The problem choosing between type-security or runtime-security is to find the golden middle ground in this case, IMO. Main problem that we are working against is to try and protect against a faulty case when the builder can run with either wrong values accidentally set to wrong positions because of matching types, or some values not set at all because they have been missed. And we must consider that we are creating the lib for the JS environment as well, not just Rust, and it's being used in non-typed JS cases as well.

Two latest changes to the parameter list both were somewhat troublesome:

1. Two new parameters for max_value_size and max_tx_size been added at the very end of the list, this did not cause any errors during tx-builder creation for people in untyped JS and also did not even fail fast in runtime, because from JS to WASM those were set to default values of zero and only were failing when later calling the build function, getting people confused. Potentially if they have been added at the beginning of the list - this would cause runtime failurs for people because i32 is being passed where LinearFee or BigNum is expected, but then not only it's not that great of a solution, but it also demonstrates that you need to remember about it every single time.

2. One the existing parameters got replaced with a completely new parameter with a different meaning and value, but same exact type. This caused the problem that even the type-check would pass for people if the parameter would be on the same position in the list. This was solved by moving the parameter to the end of the list, which nicely will fail in both the type-check and the runtime in case the old parameter-list will be used, because in runtime the BigNum would be passed in the position where i32 is now expected. But then the question: what would need to be done if the next parameter in the list were also a BigNum ? Or what will need to be done if the coins_per_utxo_word will get replaced again with a different-meaning parameter next time, but now they are at the end of the list ? Will we need to rearrange the parameter-list in weirder and weirder combinations every single time something like this happens ?

The solution with the config-builder is supposed to be the middle ground that solved more problems than it introduces:

1. The parameters are now visibly named and can be arranged in any order
2. If some parameter changes the name - it will fail fast in both the type-check (if the code uses a removed setter) and the runtime-check
3. If some new required parameters are added - it will fail fast in the runtime-check and plus protect from a default number value to be used silently from JS
4. It creates the visible and easy framework on how to add new parameters to the builder without having to think on how it will potentially cause problems in typed or untyped environments and thinking on whether you need to rearrange the parameters and how.

This protects the parameters from all sides and saves devs from having problems with adding new parameters. The only case in which it can be considered a reduction is when new required parameters are added - because now it will fail in runtime and not in type-check, but it is higher priority to at least fail fast in runtime than potentially not fail in an untyped environment at all.

[rooooooooob]

Two new parameters for max_value_size and max_tx_size been added at the very end of the list, this did not cause any errors during tx-builder creation for people in untyped JS and also did not even fail fast in runtime, because from JS to WASM those were set to default values of zero and only were failing when later calling the build function, getting people confused.

Ah, I didn't know that happened going to wasm. That makes more sense now.

[rooooooooob]

It might also be worth thinking about that since most of these are protocol parameters maybe it would make sense to treat the things that are protocol parameters (and thus will only have 1 valid setting that must be provided and won't change within a program and could be stored somewhere to be reused) differently from things that could depend based on use-case within a single program?

[vsubhuman]

We can look into, but overall it kinda sounds like a global static config to me so far, which is known to cause nothing but problems. Makes sense for a frontend app which is running in some context, but not for a library. E.g. if it would be something like:

const networkContext1 = RustModule.NetworkContextBuilder.new()
   .fee_algo(linearFee)
   .key_deposit(keyDeposit)
   ...
   .coins_per_utxo_word(coinsPerUtxoWord)
   .build()

const txBuilder1 = networkContext1.newTxBuilder();

Then this might work cuz we would just create a place where it loads that network context and then reuses it from all places. Even tho it's not much different from having a place that would similarly just load the tx-builder in a similar way.

But if you mean something like:

RustModule.set_fee_algo(linearFee);

RustModule.TransactionBuilder.new(); // knows the fee algo automatically from the global value

Then this would be a big no-no that causes nothing but pain and suffering long-term )

E.g. we should be able to create tx-builders for different network setups and different configurations even when running from a single frontend app instance, for example to support both testnet and mainet, or various additional networks, etc.

[rooooooooob]

@vsubhuman

But if you mean something like:

No, I meant more something like a separate ProtocolParemters-like struct to set those things then your builder config would like:

var protocol_params = ProtocolParams::new().coins_per_utxo_word(100).fee_algo(linearFee)...build();
// dev can now have 1 place to get their protocol params even if their non-protocol-param parameters to the tx builder vary
builder_config.protocol_params(&protocol_params).prefer_pure_change(true).build().unwrap();

So that we would separate more the protocol parameters (which at a given point in time on a chain should be the same) from the things specific just to the tx builder.

Although it depends more on how many other non-protocol-params we are adding to the builder. I guess you could have the built so far config builder with the params set saved somewhere then after #270 it copies instead of consumes so you just use that mostly-filled builder to create your specific-usage config so maybe it's not as important now that it's non-consuming.

[vsubhuman]

Ah, understood now. Yeah, I think so far there's only one non-protocol parameter so far, but this might be a really good idea for the future development if that functionality will be growing and growing further