TxBuilder: CIP2 MultiAsset variants added #264

rooooooooob · 2021-11-21T05:23:18Z

Implemented an adaption of CIP2LargestFirst to handle multiassets as
the current implementation would only order by ADA resulting in very
poor selections if many inputs were available and non-ADA multiassets
were in the outputs.

The previous CIP2LargestFirst algorithm will now error on multiasset
outputs.

MultiAsset's documentation was improved slightly and convenience methods
get_asset() and set_asset() are included now to avoid having to
additionally deal with Assets and do it all at once.

RandomImprove was also implemented for multiasset, although we might need to adapt it further or make some changes (see comments in latest commit)

Implemented an adaption of CIP2LargestFirst to handle multiassets as the current implementation would only order by ADA resulting in very poor selections if many inputs were available and non-ADA multiassets were in the outputs. The previous CIP2LargestFirst algorithm will now error on multiasset outputs. MultiAsset's documentation was improved slightly and convenience methods `get_asset()` and `set_asset()` are included now to avoid having to additionally deal with Assets and do it all at once.

rooooooooob · 2021-11-23T09:54:55Z

rust/src/tx_builder.rs

+            .collect::<Vec<TransactionOutput>>();
+        outputs.sort_by_key(|output| by(&output.amount).expect("filtered above"));
+        for output in outputs.iter().rev() {
+            // TODO: how should we adapt this to inputs being associated when running for other assets?


I'm not sure how significant this is for most wallets wrt the multiassets. We could do a halfway point and only consider this for the ADA-only stage (e.g. so we count the inputs added to satisfy the multiassets towards the ADA portion). Although it might be not much more work to just count inputs added in previous rounds (for other assets) against the outputs for subsequent rounds in general not just for ADA. It's a bit weird since random-improve is specified to do things per-output rather than for the total output required.

rooooooooob · 2021-11-24T01:29:18Z

rust/src/tx_builder.rs

-                            let move_closer = (ideal as i128 - new as i128).abs() < (ideal as i128 - cur as i128).abs();
-                            let not_exceed_max = new < max;
-                            if move_closer && not_exceed_max {
-                                std::mem::swap(input, new_input);


I think this part was broken before anyway. Maybe that's why that phase 3 was necessary despite adding in the input fee to the required amount in phase 1 before? The inputs in associated_inputs would be distinct copies from the ones in the builder as they are added above as a copy.

vsubhuman · 2021-11-24T09:01:21Z

@ozgrakkurt @pedromtcosta , plz review

rust/src/lib.rs

rust/src/tx_builder.rs

NicolasDP · 2021-11-30T08:43:23Z

rust/src/lib.rs

+    pub fn insert(&mut self, policy_id: &PolicyID, assets: &Assets) -> Option<Assets> {
+        self.0.insert(policy_id.clone(), assets.clone())


this is something I'm seeing everywhere in the rust library, why are we passing values by reference if it is to clone them. Why not make the API take by values and let the user make the right choices with the memory ?

It's for when it's consumed on the JS side via wasm, which is most of our users. Move semantics don't translate very well over to JS/wasm and there's no rust borrow checker to help you over there. If it were a rust-only library I would 100% agree though.

I think maybe we should convert all rust code to idiomatic rust code and make a js api over it so it is good to use on js side.

I see. Well this is a problem inherent to the whole rust library here. so not necessarily something to do in this P. Though I thought I'd just ask the reason and see what the answer would be.

I'm not sure how well we could simply "make a js api over it".

If we go about it by wrappers on the JS side which contain an object in wasm bound by wasm-bindgen we have these cons:

who owns the memory? how do we track that in JS? (maybe this is solvable but afaik there's no RAII/destructor we could rely on here to ref-count or whatever) simply putting it in a Rc/Arc wrapper in rust just shifts the problem since the js object that points to that itself could be copied/go out of scope/etc with no way (afaik - I could be wrong and I hope I am!) for rust to be aware of that. Maybe js_sys has something could help there though, I haven't looked much.

If we go about it by having all the data be on the JS side natively then we have the following cons:

we would have to rewrite so much of the lib in JS

we would have to write conversion code to go to/from JS to wasm for all types

this might have a significant performance penalty going over the boundary multiple times instead of just relying on ptrs to wasm linear memory where the objects exists like now.

If we go about it by having there be idiomatic rust code with separate rust wasm-bindgen annotated newtypes (js-chain libs reference. This is what IOHK did for but this made sense since the rust code was already written and they were just providing a wasm API around it) we have these cons:

experience is equally awful from JS (possibly slightly worse performance since more indirection but likely irrelevant), but it's better from rust. keep in mind most users are using the js bindings not the rust crate afaik. the npm packages have over 50x the weekly downloads as the rust crate - maybe not the best since performance matters to some projects which could be in rust and be more important than many smaller js users.

All of these also mean doc-comments are harder to auto-gen. Naively doing these wrappers by hand would mean we'd have to copy them manually (instead of wasm-bindgen exposing them automatically) which means they could get out of date easily if they're in two places. If we're doing this via a wasm-bindgen annotated wrapper in rust we could produce whatever wrapper by a macro we could probably extract them but then we're stuck with very 1:1 or formulaic bindings.

But yeah this probably isn't the place to discuss this. We've had a discussion (sparked by this PR) internally in our slack but maybe we want to discuss it in a public issue here instead.

I am opening a public issue for this

NicolasDP · 2021-11-30T08:46:07Z

rust/src/tx_builder.rs

@@ -265,118 +271,252 @@ impl TransactionBuilder {
    /// This function, diverging from CIP2, takes into account fees and will attempt to add additional
    /// inputs to cover the minimum fees. This does not, however, set the txbuilder's fee.
    pub fn add_inputs_from(&mut self, inputs: &TransactionUnspentOutputs, strategy: CoinSelectionStrategyCIP2) -> Result<(), JsError> {
-        let mut available_inputs = inputs.0.clone();
+        let available_inputs = &inputs.0.clone();


why do you clone to take a reference of the array ?

vsubhuman requested review from vsubhuman, ozgrakkurt and pedromtcosta November 21, 2021 20:28

TxBuilder: CIP2-RandomImproveMultiAsset

ca74d98

rooooooooob changed the title ~~TxBuilder: CIP2LargestFirstMultiAsset + improve MultiAsset~~ TxBuilder: CIP2 MultiAsset variants added Nov 23, 2021

rooooooooob commented Nov 23, 2021

View reviewed changes

rooooooooob commented Nov 24, 2021

View reviewed changes

rooooooooob and others added 2 commits November 23, 2021 17:50

Merge branch 'master' into cip30-multiasset-largest-first

5786f04

Fix flaky test: tx_builder_cip2_random_improve_multiasset

185abfc

ozgrakkurt reviewed Nov 25, 2021

View reviewed changes

rust/src/lib.rs Show resolved Hide resolved

ozgrakkurt reviewed Nov 25, 2021

View reviewed changes

rust/src/tx_builder.rs Show resolved Hide resolved

ozgrakkurt reviewed Nov 25, 2021

View reviewed changes

rust/src/tx_builder.rs Show resolved Hide resolved

rust/src/tx_builder.rs Show resolved Hide resolved

Update MultiAsset comments

ddd14ba

NicolasDP reviewed Nov 30, 2021

View reviewed changes

ozgrakkurt mentioned this pull request Dec 2, 2021

Current state of rust library API and implementation #276

Open

rooooooooob mentioned this pull request Dec 7, 2021

TxBuilder: Implement CIP-0002 Coin Selection #232

Merged

vsubhuman mentioned this pull request Dec 7, 2021

BETA Release / 10.0.0-beta.8 #283

Merged

This was referenced Dec 7, 2021

Bug with min_ada_required() #282

Closed

Documentation #191

Open

vsubhuman added 4 commits December 13, 2021 02:47

Merge branch 'master' into cip30-multiasset-largest-first

ae90a00

Merge branch 'master' into cip30-multiasset-largest-first

1c7538d

Merge branch 'master' into cip30-multiasset-largest-first

03609e1

Merge branch 'master' into cip30-multiasset-largest-first

c7ecdd2

vsubhuman added this to the 10.0.0 milestone Jan 23, 2022

vsubhuman approved these changes Jan 23, 2022

View reviewed changes

vsubhuman merged commit aa08824 into master Jan 23, 2022

vsubhuman deleted the cip30-multiasset-largest-first branch January 23, 2022 20:52

vsubhuman mentioned this pull request Feb 6, 2022

Release / 10.0.0 #358

Merged

40 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TxBuilder: CIP2 MultiAsset variants added #264

TxBuilder: CIP2 MultiAsset variants added #264

rooooooooob commented Nov 21, 2021 •

edited

Loading

rooooooooob Nov 23, 2021

rooooooooob Nov 24, 2021

vsubhuman commented Nov 24, 2021

NicolasDP Nov 30, 2021

rooooooooob Nov 30, 2021

ozgrakkurt Nov 30, 2021

NicolasDP Nov 30, 2021

rooooooooob Dec 1, 2021

ozgrakkurt Dec 2, 2021

NicolasDP Nov 30, 2021

		pub fn insert(&mut self, policy_id: &PolicyID, assets: &Assets) -> Option<Assets> {
		self.0.insert(policy_id.clone(), assets.clone())

TxBuilder: CIP2 MultiAsset variants added #264

TxBuilder: CIP2 MultiAsset variants added #264

Conversation

rooooooooob commented Nov 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsubhuman commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rooooooooob commented Nov 21, 2021 •

edited

Loading