Bug with min_ada_required()

[BlakeBrown]

min_ada_required() seems to report the wrong minimum ADA value for multiassets 😢

To repro:

  // Construct multiasset
  const assets = Assets.new();
  assets.insert(
    AssetName.new(
      Buffer.from("436c756d737947686f737473313331", "hex")
    ),
    BigNum.from_str("1")
  );
  const multiAsset = MultiAsset.new();
  multiAsset.insert(
    ScriptHash.from_bytes(
      Buffer.from(
        "b000e9f3994de3226577b4d61280994e53c07948c8839d628f4a425a",
        "hex"
      )
    ),
    assets
  );

  // Construct value
  const testValue = Value.new(
    BigNum.from_str("0")
  );
  testValue.set_multiasset(multiAsset);

  // Print min_ada
  console.log(
    min_ada_required(
      testValue,
      false,
      BigNum.from_str("1000000")
    ).to_str()
  );

Gives a total of 40 ADA or 40,000,000 lovelace 🤯

Any ideas why this might be happening?

[vsubhuman]

 // Print min_ada
  console.log(
    min_ada_required(
      testValue,
      false,
      BigNum.from_str("1000000")
    ).to_str()
  );

Hi, @BlakeBrown! See release draft notes for version 10, the third parameter to the min_ada_required is the new protocol parameter "coin per utxo word" which has a much lower value. You are passing the value for old parameter "min utxo value" which is removed in Cardano Alonzo era and in the upcoming version 10 of the library

[AngelCastilloB]

Hi @vsubhuman , I am assuming this refers to the change on this commit: 40565b9?

static MINIMUM_UTXO_VAL: u64 = 1_000_000;

is now

static COINS_PER_UTXO_WORD: u64 = 34_482; ?

Thanks, I am still unable to find the release draft notes for version 10 of the library.

[vsubhuman]

Thanks, I am still unable to find the release draft notes for version 10 of the library.

Ah, you just open the releases section and the draft will be visible at the top, if it exists: https://github.com/Emurgo/cardano-serialization-lib/releases

https://github.com/Emurgo/cardano-serialization-lib/releases/tag/untagged-38227e1d0ee851c64be5

[AngelCastilloB]

I can not see it, I can only see up to version 9.1.2, also the link:

https://github.com/Emurgo/cardano-serialization-lib/releases/tag/untagged-38227e1d0ee851c64be5

Goes to error 404 page not found. Maybe it is not publicly visible?

[vsubhuman]

I can not see it, I can only see up to version 9.1.2, also the link:

https://github.com/Emurgo/cardano-serialization-lib/releases/tag/untagged-38227e1d0ee851c64be5

Goes to error 404 page not found. Maybe it is not publicly visible?

My bad! I thought for some reason that the notes are visible publicly. Sorry for that.

I have created the release PR drafrt here with the notes: #283

[BlakeBrown]

Thank you so much @vsubhuman! 🙏 Will close out this issue.

I've gotten a bit further, but now I get the error missing input for some native asset after adding outputs, calling add_inputs_from() and then attempting to run add_change_if_needed(). Any ideas for this one? I'm on version ^10.0.0-beta.8 if that helps!

[vsubhuman]

I've gotten a bit further, but now I get the error missing input for some native asset after adding outputs, calling add_inputs_from() and then attempting to run add_change_if_needed(). Any ideas for this one? I'm on version ^10.0.0-beta.8 if that helps!

Which CoinSelectionStrategyCIP2 you have used in add_inputs_from()? The current version existing in beta8 does not support aseets in random-improve it only checks that there's enough ADA for each output.

There's a separate PR where assets support is added for random-improve, it's about to be merged, but not yet atm. There will be another beta published for it. In beta8 you can either select inputs manually or use largest-first strategy.

[BlakeBrown]

I saw that! I'm using largestFirst :/

[BlakeBrown]

EDIT: Found my mistake!! Was doing something really stupid. Apologies for the hassle 😅

[rooooooooob]

In general though I would strongly suggest not using the first version of LargestFirst for multiassets. We've got a PR #264 that seems to be planned to include into 10.0.0 that has a proper LargestFirstMultiasset (+ a RandomImprove version). It's likely to include too many unnecessary inputs before finding ones that satisfy particular assets.

[BlakeBrown]

In general though I would strongly suggest not using the first version of LargestFirst for multiassets. We've got a PR #264 that seems to be planned to include into 10.0.0 that has a proper LargestFirstMultiasset (+ a RandomImprove version). It's likely to include too many unnecessary inputs before finding ones that satisfy particular assets.

Thanks for the update @rooooooooob ! Will keep an eye open for when this gets merged 👀

[BlakeBrown]

@rooooooooob @vsubhuman

Wanted to call out a potential problem, in case you're not already aware of it.

If we start using add_inputs_from() in 10.0.0 of the library, there's currently no way of determining the index of a script input after coin selection is finished. This makes it impossible to add a redeemer to the transaction, since we need to know the index of the script input. Would it be possible to add this to the spec for 10.0.0?

I believe Alessandro made a custom library where he added the following method to the TransactionBuilder.

    /**
    * @param {TransactionInput} input
    * @returns {number}
    */
    index_of_input(input) {
        _assertClass(input, TransactionInput);
        var ret = wasm.transactionbuilder_index_of_input(this.ptr, input.ptr);
        return ret >>> 0;
    }

Would be fantastic to have this included in the official library 😄

[BlakeBrown]

Argh I hit another wall...

Seems the latest version of the library doesn't support script inputs yet. Is there any ETA on when this might be added?

[blockchainUni]

@rooooooooob @vsubhuman

Wanted to call out a potential problem, in case you're not already aware of it.

If we start using add_inputs_from() in 10.0.0 of the library, there's currently no way of determining the index of a script input after coin selection is finished. This makes it impossible to add a redeemer to the transaction, since we need to know the index of the script input. Would it be possible to add this to the spec for 10.0.0?

I believe Alessandro made a custom library where he added the following method to the TransactionBuilder.

    /**
    * @param {TransactionInput} input
    * @returns {number}
    */
    index_of_input(input) {
        _assertClass(input, TransactionInput);
        var ret = wasm.transactionbuilder_index_of_input(this.ptr, input.ptr);
        return ret >>> 0;
    }

Would be fantastic to have this included in the official library smile

Hi Blake did you figure this out
?