Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TxBuilder: CIP2 MultiAsset variants added #264

Merged
merged 9 commits into from
Jan 23, 2022
55 changes: 50 additions & 5 deletions rust/pkg/cardano_serialization_lib.js.flow
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,13 @@ declare export function encode_json_str_to_native_script(
schema: number
): NativeScript;

/**
* @param {Transaction} tx
* @param {LinearFee} linear_fee
* @returns {BigNum}
*/
declare export function min_fee(tx: Transaction, linear_fee: LinearFee): BigNum;

/**
* @param {Uint8Array} bytes
* @returns {TransactionMetadatum}
Expand Down Expand Up @@ -336,6 +343,16 @@ declare export var StakeCredKind: {|
declare export var CoinSelectionStrategyCIP2: {|
+LargestFirst: 0, // 0
+RandomImprove: 1, // 1
+LargestFirstMultiAsset: 2, // 2
+RandomImproveMultiAsset: 3, // 3
|};

/**
*/

declare export var StakeCredKind: {|
+Key: 0, // 0
+Script: 1, // 1
|};

/**
Expand Down Expand Up @@ -2620,30 +2637,58 @@ declare export class MultiAsset {
static new(): MultiAsset;

/**
* the number of unique policy IDs in the multiasset
* @returns {number}
*/
len(): number;

/**
* @param {ScriptHash} key
* @param {Assets} value
* set (and replace if it exists) all assets with policy {policy_id} to a copy of {assets}
* @param {ScriptHash} policy_id
* @param {Assets} assets
* @returns {Assets | void}
*/
insert(key: ScriptHash, value: Assets): Assets | void;
insert(policy_id: ScriptHash, assets: Assets): Assets | void;

/**
* @param {ScriptHash} key
* all assets under {policy_id}, if any exist, or else None (undefined in JS)
* @param {ScriptHash} policy_id
* @returns {Assets | void}
*/
get(key: ScriptHash): Assets | void;
get(policy_id: ScriptHash): Assets | void;

/**
* sets the asset {asset_name} to {value} under policy {policy_id}
* returns the previous amount if it was set, or else None (undefined in JS)
* @param {ScriptHash} policy_id
* @param {AssetName} asset_name
* @param {BigNum} value
* @returns {BigNum | void}
*/
set_asset(
policy_id: ScriptHash,
asset_name: AssetName,
value: BigNum
): BigNum | void;

/**
* returns the amount of asset {asset_name} under policy {policy_id}
* If such an asset does not exist, 0 is returned.
* @param {ScriptHash} policy_id
* @param {AssetName} asset_name
* @returns {BigNum}
*/
get_asset(policy_id: ScriptHash, asset_name: AssetName): BigNum;

/**
* returns all policy IDs used by assets in this multiasset
* @returns {ScriptHashes}
*/
keys(): ScriptHashes;

/**
* removes an asset from the list if the result is 0 or less
* does not modify this object, instead the result is returned
* @param {MultiAsset} rhs_ma
* @returns {MultiAsset}
*/
Expand Down
27 changes: 22 additions & 5 deletions rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2537,7 +2537,7 @@ pub type PolicyID = ScriptHash;
pub type PolicyIDs = ScriptHashes;

#[wasm_bindgen]
#[derive(Clone, Debug, Eq, Ord, PartialEq, PartialOrd)]
#[derive(Clone, Debug, Default, Eq, Ord, PartialEq, PartialOrd)]
pub struct Assets(pub (crate) std::collections::BTreeMap<AssetName, BigNum>);

to_from_bytes!(Assets);
Expand Down Expand Up @@ -2577,23 +2577,40 @@ impl MultiAsset {
Self(std::collections::BTreeMap::new())
}

/// the number of unique policy IDs in the multiasset
pub fn len(&self) -> usize {
self.0.len()
}

pub fn insert(&mut self, key: &PolicyID, value: &Assets) -> Option<Assets> {
self.0.insert(key.clone(), value.clone())
/// set (and replace if it exists) all assets with policy {policy_id} to a copy of {assets}
pub fn insert(&mut self, policy_id: &PolicyID, assets: &Assets) -> Option<Assets> {
self.0.insert(policy_id.clone(), assets.clone())
Comment on lines +2586 to +2587
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am opening a public issue for this

}

pub fn get(&self, key: &PolicyID) -> Option<Assets> {
self.0.get(key).map(|v| v.clone())
/// all assets under {policy_id}, if any exist, or else None (undefined in JS)
pub fn get(&self, policy_id: &PolicyID) -> Option<Assets> {
self.0.get(policy_id).map(|v| v.clone())
}

/// sets the asset {asset_name} to {value} under policy {policy_id}
/// returns the previous amount if it was set, or else None (undefined in JS)
pub fn set_asset(&mut self, policy_id: &PolicyID, asset_name: &AssetName, value: BigNum) -> Option<BigNum> {
self.0.entry(policy_id.clone()).or_default().insert(asset_name, &value)
}

/// returns the amount of asset {asset_name} under policy {policy_id}
/// If such an asset does not exist, 0 is returned.
pub fn get_asset(&self, policy_id: &PolicyID, asset_name: &AssetName) -> BigNum {
ozgrakkurt marked this conversation as resolved.
Show resolved Hide resolved
(|| self.0.get(policy_id)?.get(asset_name))().unwrap_or(BigNum::zero())
}

/// returns all policy IDs used by assets in this multiasset
pub fn keys(&self) -> PolicyIDs {
ScriptHashes(self.0.iter().map(|(k, _v)| k.clone()).collect::<Vec<PolicyID>>())
}

/// removes an asset from the list if the result is 0 or less
/// does not modify this object, instead the result is returned
pub fn sub(&self, rhs_ma: &MultiAsset) -> MultiAsset {
let mut lhs_ma = self.clone();
for (policy, assets) in &rhs_ma.0 {
Expand Down
Loading