`TradableKitty` piece #171

NadigerAmit · 2024-02-12T08:19:58Z

Enhanced Kitty piece with 'name' field, extended FreeKittyConstraintChecker, and introduced TradableKitty for minting, property updates, and buying in the blockchain.

Overall Summary:

Enhanced the Kitty piece by adding a 'name' field to the KittyData structure.
Extended FreeKittyConstraintChecker to support Mint and Breed functionalities.
Introduced a new TradableKitty piece with minting, property updating, and buying capabilities.

Details:
1. KittyData Enhancement:
The KittyData structure now includes a 'name' field, allowing for the storage and retrieval of kitty names.

FreeKittyConstraintChecker Extension in kitties piece: FreeKittyConstraintChecker has been expanded to support both existing Breed functionality and the newly added Mint functionality.

Introduction of new piece TradableKitty:
A new piece, TradableKitty, has been introduced.
Functionalities include minting kitties, updating properties (name, price, availability for sale), and enabling buy operations.

Integration with Existing Pieces:
TradableKitty seamlessly integrates with both existing kitties and the Money piece, leveraging their functionalities.

This related to #166

…hecker, and introduced TradableKitty for minting, property updates, and buying in the blockchain. Overall Summary: 1. Enhanced the Kitty piece by adding a 'name' field to the KittyData structure. 2. Extended FreeKittyConstraintChecker to support Mint and Breed functionalities. 3. Introduced a new TradableKitty piece with minting, property updating, and buying capabilities. Details: KittyData Enhancement: The KittyData structure now includes a 'name' field, allowing for the storage and retrieval of kitty names. FreeKittyConstraintChecker Extension in kitties piece : FreeKittyConstraintChecker has been expanded to support both existing Breed functionality and the newly added Mint functionality. Introduction of new piece TradableKitty: A new piece, TradableKitty, has been introduced. Functionalities include minting kitties, updating properties (name, price, availability for sale), and enabling buy operations. Integration with Existing Pieces: TradableKitty seamlessly integrates with both existing kitties and the Money piece, leveraging their functionalities.

…go.toml Updated PKG nam and added better description for tradable_kitties cargo.toml

coax1d

I am quite pleased to see you made this happen!!!

My main concerns are more about the implementation details. I think there is much too much repetition between kitties piece money piece and the composable tradable_kitties.

I think if we can find a way to refactor this to allow alot less repetition than this should be ready.

@JoshOrndorff What do you think?

wardrobe/kitties/src/lib.rs

wardrobe/kitties/src/tests.rs

wardrobe/tradable_kitties/src/lib.rs

wardrobe/tradable_kitties/src/tests.rs

JoshOrndorff

This is heading in a good direction. You've learned a lot already and this is a good start. I think the biggest feedback which is similar to what Andrew said is that it is too much copying the kitty code rather than reusing it. I've made a specific suggestion for how to improve this in my review comments.

Also the code quality ci is not passing. For starters you need to run cargo fmt. There is also the toml sort, but I will do that one for you for now.

wardrobe/tradable_kitties/Cargo.toml

wardrobe/tradable_kitties/src/lib.rs

Implement design change and addressed all the review comments. Mainly: 1. Now kitties can updated to tradable kitties and vice versa based on listForSale and DelistFromsale constraint checkers. 2. Support multiple inputs and outputs 3. Remove the is_avilabel_for_sale from tradable kitty 4. Add listForSale , DelistFromSale and Update name and updatePrice for tradable kitty 5. Add Update name for kitty piece.

submitted the fmt fix cargo fmt --all

wardrobe/kitties/src/lib.rs

…ut txns Implemented below : 1. Removed the out-of-order b/w input & output for Implemented the in-order for below : 1. ListKittyForSale 2. DelistKittyForSale 3. UpdateName 4.UpdatePrice For Breed operation, only 1 tradable kitty can be traded at the same time in the same txn.

JoshOrndorff

This is coming along nicely. I'm pretty picky about typos and grammar stuff in comments, so please look that stuff over carefully. Also make sure to run rustfmt.

This is getting close.

tuxedo-template-runtime/src/lib.rs

wardrobe/kitties/src/lib.rs

JoshOrndorff · 2024-03-01T14:29:05Z

wardrobe/kitties/src/lib.rs

+        input_data.len() == output_data.len() && !input_data.is_empty(),
+        { ConstraintCheckerError::InvalidNumberOfInputOutput }
+    );
+    let mut dna_to_kitty_set: BTreeSet<KittyDNA> = BTreeSet::new();


It seems like you're spending a lot of effort making sure that no two kitties have the same DNA here. But isn't that already guaranteed (assuming polynomial time advarsaries) by the hash function itself?

I recommend you remove this check entirely. Here you should just make sure that the output kitties are the same as the input kitties except for the names. If you aren't convinced, I challenge you to think of a case where two kitties would have the same DNA.

@JoshOrndorff, Thank you.

I can remove those checks but I thought of below cases:
Good wallets don't abuse the blockchain apis, but rouge wallets can abuse the blockchain API as below :

Case1:

I have 1 kitty in my wallet

I will send the same kitty 2 times as below :
let inputs: Vec<Input> = vec![input_kitty_ref.clone(),input_kitty_ref]; // I am just sending same kitty 2 times

Then I will create the 2 outputs as below in the wallet :

let mut updated_kitty: KittyData = input_kitty_info.clone(); // This is valid kitty updated_kitty.name = *kitty_name; let output = Output { payload: updated_kitty.into(), verifier: OuterVerifier::Sr25519Signature(Sr25519Signature { owner_pubkey: args.owner, }), }; let mut updated_kitty_fake: KittyData = input_kitty_info; // This is fake kitty updated_kitty_fake.dna = KittyDNA(H256::from_slice(b"mom_kitty_1asdfasdfasdfasdfasdfz")); // Here I am updating the kitty DNA to some random value. updated_kitty_fake.name = *fake_kitty_name; // name as lily let output1 = Output { payload: updated_kitty_fake.into(), verifier: OuterVerifier::Sr25519Signature(Sr25519Signature { owner_pubkey: args.owner, }), }; // Create the Transaction let transaction = Transaction { inputs: inputs, peeks: Vec::new(), outputs: vec![output,output1], // Including the both valid and fake kitty checker: FreeKittyConstraintChecker::UpdateKittyName.into(), }; transaction }

If I don't have DNA comparison between input and output this I think it is valid transaction:
Below is o/p when I tested.

Blockchain logs: I truncated the logs manually for easy reading :
INFO tokio-runtime-worker tuxedo_template_runtime: runtime-> validate_transaction TransactionSource = TransactionSource::InBlock tx = Transaction { inputs: [Input { output_ref: OutputRef { tx_hash: 0x928ff2975393c8466ef76121f98692a3500f1a4579372799eaffb27deaf2b387, index: 0 }, redeemer: [184, ---] }, verifier: Sr25519Signature(Sr25519Signature { owner_pubkey: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 }) }, Output { payload: DynamicallyTypedData { data: [0, --- 116, 116] }, verifier: Sr25519Signature(Sr25519Signature { owner_pubkey: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 }) }], checker: FreeKitty(UpdateKittyName) } block_hash = 0x37040850b24d9dad724f1657ea572b1536df1c7fdf52670090dd54b4a0b6c537

Below is the o/p of the wallet

You can see I could create 2 kitties with 1 kitty by abusing update name API .

amit@DESKTOP-TF687VE:~/OmBlockchain/OmTest$ ./target/release/tuxedo-template-wallet show-all-kitties Show All Kitty Summary ========================================== Owner -> 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 Some("amit") => KittyData { parent: Mom(RearinToGo), free_breedings: 2, dna: KittyDNA(0xe23edbf55798fbda8f91204651446243fc675e63e10fe126fdff073755547dc3), num_breedings: 3, name: [97, 109, 105, 116] } -> -------------------------------------------------- =-=================================================== // Updating the kitty, From cmd line, updating only 1 kitty, but I have hacking code in the source code =-=================================================== amit@DESKTOP-TF687VE:~/OmBlockchain/OmTest$ ./target/release/tuxedo-template-wallet update-kitty-name --current-name "amit" --new-name "vina" [2024-03-02T06:03:39Z INFO tuxedo_template_wallet] cli from cmd args = Cli { endpoint: "http://localhost:9944", path: None, no_sync: false, tmp: false, dev: false, command: Some(UpdateKittyName(UpdateKittyNameArgs { current_name: "amit", new_name: "vina", owner: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 })) } Node's response to spawn transaction: Ok("0xa2b0a4a144115e15df280ecd21bcfa7f13523998f10fadec856d3b64b65aad87") Created "bd3f0a8bb0596f336c82122cd373b2743717d28d82bdf3ee48a0b86d458c0f7100000000" basic Kitty 0xe23edbf55798fbda8f91204651446243fc675e63e10fe126fdff073755547dc3. owned by 0xd2bf…df67 Created "bd3f0a8bb0596f336c82122cd373b2743717d28d82bdf3ee48a0b86d458c0f7101000000" basic Kitty 0x6d6f6d5f6b697474795f3161736466617364666173646661736466617364667a. owned by 0xd2bf…df67 amit@DESKTOP-TF687VE:~/OmBlockchain/OmTest$ ./target/release/tuxedo-template-wallet show-all-kitties Show All Kitty Summary ========================================== Owner -> 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 Some("vina") => KittyData { parent: Mom(RearinToGo), free_breedings: 2, dna: KittyDNA(0xe23edbf55798fbda8f91204651446243fc675e63e10fe126fdff073755547dc3), num_breedings: 3, name: [118, 105, 110, 97] } -> -------------------------------------------------- Owner -> 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 Some("lily") => KittyData { parent: Mom(RearinToGo), free_breedings: 2, dna: KittyDNA(0x6d6f6d5f6b697474795f3161736466617364666173646661736466617364667a), num_breedings: 3, name: [108, 105, 108, 121] } ->

So thought of preventing that with the Line no 647 to 649 i.e below :
if utxo_input_kitty.dna != utxo_output_kitty.dna { return Err(ConstraintCheckerError::DnaMismatchBetweenInputAndOutput); }

With the above check the transaction will fail as below: I removed some logs for easy reading .
TransactionSource = TransactionSource::External tx = Transaction { inputs: [Input { output_ref: OutputRef { tx_hash: 0x45ce7924543b5e60490c93e25ff8b379589290d90a46df1725bf4e57d9325fb2, index: 0 }, redeemer: [---0, 108, 105, 108, 121], type_id: [75, 105, 116, 116] }, verifier: Sr25519Signature(Sr25519Signature { owner_pubkey: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 }) }], checker: FreeKitty(UpdateKittyName) } block_hash = 0x86ed44adb9742d6cebea1538bab2645812a00e3b11249dc44379ec957fcd1629 2024-03-02 11:22:04.243 WARN tokio-runtime-worker tuxedo-core: Tuxedo Transaction did not validate (in the pool): ConstraintCheckerError(FreeKitty(DnaMismatchBetweenInputAndOutput))

Case 2:
If I send 2 output kittyes with same DNA i.e without below line in wallet:

updated_kitty_fake.dna = KittyDNA(H256::from_slice(b"mom_kitty_1asdfasdfasdfasdfasdfz")); // Here I am updating the kitty DNA to some random value.

I did the below steps
1. The code is the same as 1st case except for the DNA update of updated_kitty_fake i.e I didn't do a DNA update.
2. Update the kitty name from vina to Tina.
3. Then I have a new kitty lily which has the same DNA as the updated kitty tina . Then also transaction validation is passed without those checks;

Below are the wallet logs

amit@DESKTOP-TF687VE:~/OmBlockchain/OmTest$ ./target/release/tuxedo-template-wallet update-kitty-name --current-name "vina" --new-name "tina" Node's response to spawn transaction: Ok("0x9289fc86d74f341a624c0cb5df161a583b513ac53efdcf8f271f844de200f454") Created "12c23c992f25e080e4954b76ed0c6dfb02449e92e1c28eab686c42a8072fe72100000000" basic Kitty 0xe23edbf55798fbda8f91204651446243fc675e63e10fe126fdff073755547dc3. owned by 0xd2bf…df67 Created "12c23c992f25e080e4954b76ed0c6dfb02449e92e1c28eab686c42a8072fe72101000000" basic Kitty 0xe23edbf55798fbda8f91204651446243fc675e63e10fe126fdff073755547dc3. owned by 0xd2bf…df67 amit@DESKTOP-TF687VE:~/OmBlockchain/OmTest$ ./target/release/tuxedo-template-wallet show-all-kitties Show All Kitty Summary ========================================== Owner -> 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 Some("tina") => KittyData { parent: Mom(RearinToGo), free_breedings: 2, dna: KittyDNA(0xe23edbf55798fbda8f91204651446243fc675e63e10fe126fdff073755547dc3), num_breedings: 3, name: [116, 105, 110, 97] } -> -------------------------------------------------- Owner -> 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 Some("lily") => KittyData { parent: Mom(RearinToGo), free_breedings: 2, dna: KittyDNA(0xe23edbf55798fbda8f91204651446243fc675e63e10fe126fdff073755547dc3), num_breedings: 3, name: [108, 105, 108, 121] } -> -------------------------------------------------- Owner -> 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 Some("lily") => KittyData { parent: Mom(RearinToGo), free_breedings: 2, dna: KittyDNA(0x6d6f6d5f6b697474795f3161736466617364666173646661736466617364667a), num_breedings: 3, name: [108, 105, 108, 121] }

Blockchain log:
2024-03-02 11:38:54.004 INFO tokio-runtime-worker tuxedo_template_runtime: runtime-> apply_extrinsic Transaction { inputs: [Input { output_ref: OutputRef { tx_hash: 0xbd3f0a8bb0596f336c82122cd373b2743717d28d82bdf3ee48a0b86d458c0f71, index: 0 }, redeemer: [56, 5, 230, 157, 51, 235, 0, 59, 119, 144, 251, 58, 29, 247, 215, 194, 194, 241, 15, 174, 193, 227, 100, 185, 51, 92, 123, 148, 120, 6, 81, 31, 243, 141, 56, 37, 230, 43, 233, 254, 225, 60, 25, 119, 206, 8, 84, 229, 145, 92, 46, 128, 123, 90, 234, 63, 48, 98, 198, 238, 7, 178, 77, 129] }, Input { output_ref: OutputRef { tx_hash: 0xbd3f0a8bb0596f336c82122cd373b2743717d28d82bdf3ee48a0b86d458c0f71, index: 0 }, redeemer: [172, 171, 253, 111, 104, 241, 143, 143, 59, 122, 103, 39, 113, 118, 69, 21, 56, 136, 207, 121, 143, 84, 229, 76, 146, 230, 46, 106, 240, 146, 252, 69, 149, 254, 28, 98, 163, 118, 28, 100, 185, 176, 226, 204, 235, 182, 67, 31, 83, 211, 10, 230, 200, 86, 142, 38, 176, 239, 255, 252, 170, 164, 128, 141] }], peeks: [], outputs: [Output { payload: DynamicallyTypedData { data: [0, -- 121], type_id: [75, 105, 116, 116] }, verifier: Sr25519Signature(Sr25519Signature { owner_pubkey: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 }) }], checker: FreeKitty(UpdateKittyName) } 2024-03-02 11:38:54.006 INFO tokio-runtime-worker tuxedo_template_runtime: runtime-> finalize_block

If I have the below line I can avoid this situation as inputs are checked.
if dna_to_kitty_set.contains(&utxo_input_kitty.dna) { return Err(ConstraintCheckerError::DuplicateKittyFound);

I checked the kitties via verifykitty which will get the kitties from the blockchain db, Then I also could fine 2 kitties with the same DNA.

If I am doing something wrong in the above tests, Please suggest.

@JoshOrndorff

Hi Joshy,

I have delved deeper into the issues and identified the root cause of why duplicate DNA can occur in kitties.
But basically I think the issue is common to any UTXOs.

Issue 1: Duplicate UTXO Checks
Presently, our UTXO validation primarily focuses on the existing UTXOs within the blockchain database i.e 'TransparentUtxoSet', detecting any duplicate output UTXOs from the transaction output sets.
If a duplicate UTXO is detected, we trigger the error 'PreExistingOutput.' However, it seems we do not perform checks for duplicity within the outputs of the transaction itself. I think it may be valid in the case of coin/money.

Tuxedo/tuxedo-core/src/executive.rs

Lines 110 to 125 in 1ad2343

for index in 0..transaction.outputs.len() {

let output_ref = OutputRef {

tx_hash,

index: index as u32,

};

debug!(

target: LOG_TARGET,

"Checking for pre-existing output {:?}", output_ref

);

ensure!(

TransparentUtxoSet::<V>::peek_utxo(&output_ref).is_none(),

UtxoError::PreExistingOutput

);

}

Perhaps originally, the design intended to have a single output of the same type.
Due to this, I can create identical kitties by cloning in the 'create_kitty' transaction, and all the outputs are validated and stored in the database as valid UTXOs.

Possible Solutions:
Solution 1: Consider supporting only one output of the same type at a time or per transaction to address the issue of allowing multiple cloned outputs as valid transactions.
Solution 2: Implement a mechanism in each piece to verify uniqueness within the output UTXO, preventing the inclusion of duplicate outputs in a transaction.

Issue 2: DNA Calculation and Uniqueness

This issue predominantly concerns kitties, and it remains unclear if it also applies to other UTXOs. Currently,
we calculate DNA based on the 'dna_preimage' supplied by the client. While we can identify duplicate DNA if the 'dna_preimage' is the same and the other information is identical, considering the hash of the UTXO as a whole, this becomes challenging when parameters like Parents (gender, name, etc.) can be updated, altering the hash of the UTXO and compromising DNA uniqueness.
This issues is exploited in any of the transactions like updateName , UpdatePrice, listKittyForSAle,etc

Possible Solution:
To address this, we can reconsider the DNA calculation process it self or check the uniqueness of DNA within the Kitty piece, as we are currently doing.

I think it is better to create new issues for this. I will do it

I've replied in your issue #190. I think you are not distinguishing between the concepts of two kitties with the same DNA vs a single kitty. You cannot input the same kitty twice because tuxedo core already checks it.

The question here is how would you ever get two identical kitties to begin with. The only way is if you mint them directly. Up to you whether that is valid or not. If it is not, use a universal creator to ensure they are always unique. If it is valid to mint kitties with identical dna, then it should be no surprise that their offspring can also have identical DNA.

Thanks @JoshOrndorff, I removed the DNA check.

wardrobe/tradable_kitties/src/lib.rs

wardrobe/tradable_kitties/src/tests.rs

Code Improvement

Implement review comments : Main updates : 1. Price of kitty of mage mandatory. 2. DNA duplicate checks removed. 3. Documentation is revisited. 4. Test cases are updated.

Fixed clippy issues

muraca

I have currently reviewed the Kitties piece only, I will take a look at TradableKitties later

wardrobe/kitties/src/lib.rs

muraca · 2024-03-08T11:28:01Z

wardrobe/kitties/src/lib.rs

+            Self::Create => {
+                // Ensure that no inputs are being consumed.
+                ensure!(
+                    input_data.is_empty(),
+                    ConstraintCheckerError::CreatingWithInputs
+                );
+
+                // Ensure that at least one kitty is being created.
+                ensure!(
+                    !output_data.is_empty(),
+                    ConstraintCheckerError::CreatingNothing
+                );
+
+                // Ensure the outputs are the right type.
+                for utxo in output_data {
+                    let _utxo_kitty = utxo
+                        .extract::<KittyData>()
+                        .map_err(|_| ConstraintCheckerError::BadlyTyped)?;
+                }
+                Ok(0)


Should we somehow check that a Kitty with the same DNA does not exist?
cc @JoshOrndorff

This is one of the reasons I didn't like minting kitties from scratch

I raised this question earlier with Joshy and found that no need to check for duplicate DNA check since there can be twin kitties with duplicate DNA. So I removed the duplicate DNA check.

Well, we state in multiple parts of the documentation that DNA is unique 😄

As far as I remember, the twins use-case was not initially part of Kitties, what's the reason behind adding it?

Moreover, twins in my opinion should not have the exact same DNA, as it is also in real life

@muraca
Please see the discussion here: #171 (comment)
Earlier I implemented the DNA check for create operation and other operations also.
There were 2 options from Joshy either "universal creator pattern" or allow duplicate DNA : I chose the 2nd option i.e allow duplicate DNA kitties.

On a call recently, I encouraged @NadigerAmit to design the game fully before trying to build it and get a PR approved. Specifically I encouraged him to consider:

Who is going to be able to mint in production? Only a centralized team? Or any rando? How will minting be sybil resistant?

Should every kitty have unique DNA? If so implement a universal creator pattern as shown in NFT with Royalties piece #185 .

I don't think there are right vs wrong answers. But you need a design and you need to be consistent about it. I worry we reached a point where Amit feels very "close" to getting this PR merged, but I feel the design work isn't even done to compare the code against.

wardrobe/kitties/src/lib.rs

muraca · 2024-03-08T11:30:20Z

wardrobe/kitties/src/lib.rs

+    ensure!(
+        original_kitty.dna == updated_kitty.dna,
+        ConstraintCheckerError::DnaMismatchBetweenInputAndOutput
+    );
+    ensure!(
+        original_kitty != updated_kitty,
+        ConstraintCheckerError::KittyNameUnAltered
+    );
+    ensure!(
+        original_kitty.free_breedings == updated_kitty.free_breedings,
+        ConstraintCheckerError::FreeBreedingCannotBeUpdated
+    );
+    ensure!(
+        original_kitty.num_breedings == updated_kitty.num_breedings,
+        ConstraintCheckerError::NumOfBreedingCannotBeUpdated
+    );
+    ensure!(
+        original_kitty.parent == updated_kitty.parent,
+        ConstraintCheckerError::KittyGenderCannotBeUpdated
+    );


@JoshOrndorff do you think we should encourage this approach of multiple errors and verbosity?

I believe it is preferable to provide a more granular error message, specifying the reason for the issue. High-level errors like 'BasicPropertiesAltered' may not offer sufficient information to developers, especially when dealing with multiple properties, making it challenging to discern the exact nature of the problem.

I'm not necessarily against this, but I think there should be some clear guidelines in Tuxedo, as sometimes we did the opposite

I don't have an opinion. I'm fine to try it out this way and see what is better for downstream devs and end users.

wardrobe/tradable_kitties/src/lib.rs

muraca · 2024-03-08T13:24:28Z

wardrobe/tradable_kitties/src/lib.rs

+/// The default price of a kitten is 10.
+const DEFAULT_KITTY_PRICE: u128 = 10;


Do we really need to put a const here? We never use that, if not in the default method, where we could just hard-code the value.

Moreover, we should not make assumptions on the currency unit's order of magnitude. This number might be a lot for some, while worthless for others. I think the right approach is to use 0.

I believe using const for defining constants is generally preferable over hardcoding values directly. This is especially true for values meant to remain constant and not change during runtime. const aids in achieving "Readability and Intention," a "Single Source of Truth," and also facilitates static type checking.

Although it is not used elsewhere, we might use it later to compare if the values are the same as the default value or if they have been changed by the user.

When I mention "10," I mean 10 coins; the unit is not assumed here. The value of the coin can be anything, and currently, coins do not have any units.

Initially, I considered using 0 as the default value, but I opted for 10 for the following reasons:

When listing a kitty for sale, the kitty should not have 0 as the price.
Allowing a 0 price for a kitty could lead to a scenario where a buyer sends a buy transaction immediately after the seller lists the kitty for sale but before updating the price to some value. In such a case, the transaction would be successful, and the kitty would be sold at a 0 price.

Quick question, why do we need to implement Default for a TradableKitty?

Anyhow, let me try to explain the concept of currency unit.

Say we have a currency called TUX which only has 2 decimals, then 1 TUX is represented as 100u128.

On Kusama we have 12 decimals, therefore 1 KSM is represented as 1_000_000_000_000u128 (source).

The number 10u128 has completely different values for the two currencies.

@muraca Thanks for the explanation. In the beginning, users had the option to mint or create Tradable Kitties. At that time, providing default parameters for creating Tradable Kitties seemed logical, making them easier to use in wallets. However, I now we can simply convert a basic Kitty into a Tradable Kitty. Consequently, it's safe to remove the default implementation altogether for tradable kitty.

The best place for "constants" like this is a config trait. Then they are configurable at the runtime level and remain constant up to the point of a runtime upgrade. But they are not hard-coded and constant across chains and tokens.

wardrobe/tradable_kitties/src/lib.rs

muraca · 2024-03-08T14:00:23Z

wardrobe/tradable_kitties/src/lib.rs

+    for coin_data in input_data.iter().skip(1) {
+        let coin = coin_data
+            .clone()
+            .extract::<Coin<ID>>()
+            .map_err(|_| TradeableKittyError::BadlyTyped)?;
+
+        let utxo_value = coin.0;
+        ensure!(
+            utxo_value > 0,
+            TradeableKittyError::MoneyError(MoneyError::ZeroValueCoin)
+        );
+        input_coin_data.push(coin_data.clone());
+        total_input_amount = total_input_amount
+            .checked_add(utxo_value)
+            .ok_or(TradeableKittyError::MoneyError(MoneyError::ValueOverflow))?;
+    }


This might be a little bias of mine but I like the big Rust one-liners instead of instantiating empty Vecs and pushing each time, which is also way less efficient.
Same below

The above code is clear and easy to understand. It makes the code maintainable and understandable by myself and others.I would like to keep the implementation as it is.
If you have strong request, Please let me know I will change it to oneliner.

I prefer chaining iterator methods in most cases, but anything that passes clippy and isn't ridiculous is welcome here.

wardrobe/tradable_kitties/src/lib.rs

muraca · 2024-03-08T14:02:47Z

wardrobe/tradable_kitties/src/lib.rs

+/// Wrapper function for verifying the conversion from basic kitties to tradable kitties.
+/// Multiple kitties can be converted in a single transaction.
+fn check_can_list_kitty_for_sale(
+    input_data: &[DynamicallyTypedData],
+    output_data: &[DynamicallyTypedData],
+) -> Result<TransactionPriority, TradeableKittyError> {
+    check_kitty_tdkitty_interconversion(input_data, output_data)?;
+    Ok(0)
+}
+
+/// Wrapper function for verifying the conversion from tradable kitties to basic kitties.
+/// Multiple tradable kitties can be converted in a single transaction.
+fn check_can_delist_kitty_from_sale(
+    input_data: &[DynamicallyTypedData],
+    output_data: &[DynamicallyTypedData],
+) -> Result<TransactionPriority, TradeableKittyError> {
+    // Below is the conversion from tradable kitty to regular kitty, the reverse of the ListKittyForSale.
+    // Hence, input parameters are reversed.
+    check_kitty_tdkitty_interconversion(output_data, input_data)?;
+    Ok(0)
+}


I don't like these wrapper functions at all

I think the advantage of the wrapper functions in this case is code reuse and clarity. By using the same underlying function check_kitty_tdkitty_interconversion in both wrapper functions with slightly different parameter orders, I think the code is more concise and easier to understand. The wrappers provide a clear and consistent interface for checking the conversion of kitties in different scenarios (listing for sale and delisting from sale), reducing redundancy and improving maintainability.

You can just write a good description for check_kitty_tdkitty_interconversion (and a better name 😄) stating what the function does based on the order of parameters, and a small comment before calling the function to explain why you called that with that order of parameters

@muraca, Ok I will remove the wrapper function.

wardrobe/tradable_kitties/src/lib.rs

Fixed review comments from Matteo Muraca

Web service implementation

This reverts commit 9c4f903.

JoshOrndorff · 2024-04-22T16:03:37Z

@NadigerAmit What are your plans with this PR. Shall I take another pass at reviewing it? Or are you guys working in your own repo now?

If you are in your own repo, please give us a link so others who find this PR know where to continue looking.

NadigerAmit · 2024-04-27T01:51:47Z

@JoshOrndorff ,
Thanks. Below is the link to where we updated the kitty piece and the newly implemented tradable kitty piece.
Also, a wallet supports the below kitty functionalities :

Create_kitty
Update kitty name
List kitty for sale
Update tradable kitty name
Update tradable kitty price
Buy kitty
Breed kitty
De-list kitty from sale.

In addition to the above "#62" is also supported.

Link: https://github.com/mlabs-haskell/Tuxedo

BR,
Amit

NadigerAmit added 2 commits February 12, 2024 13:40

Updated PKG nam and added better description for tradable_kitties car…

2e0885d

…go.toml Updated PKG nam and added better description for tradable_kitties cargo.toml

coax1d reviewed Feb 13, 2024

View reviewed changes

JoshOrndorff requested changes Feb 13, 2024

View reviewed changes

toml sort

80d2e99

NadigerAmit commented Feb 14, 2024

View reviewed changes

wardrobe/tradable_kitties/src/lib.rs Outdated Show resolved Hide resolved

JoshOrndorff reviewed Feb 14, 2024

View reviewed changes

wardrobe/tradable_kitties/src/lib.rs Outdated Show resolved Hide resolved

NadigerAmit added 3 commits February 22, 2024 15:38

Merge branch 'main' of https://github.com/NadigerAmit/Tuxedo

eea8241

Submit the fmt fix

1f5855b

submitted the fmt fix cargo fmt --all

JoshOrndorff reviewed Feb 23, 2024

View reviewed changes

wardrobe/kitties/src/lib.rs Outdated Show resolved Hide resolved

JoshOrndorff reviewed Mar 1, 2024

View reviewed changes

NadigerAmit added 3 commits March 1, 2024 21:02

Code improvement

32be05f

Code Improvement

Implement the review comments

7b0a12e

Implement review comments : Main updates : 1. Price of kitty of mage mandatory. 2. DNA duplicate checks removed. 3. Documentation is revisited. 4. Test cases are updated.

Fix clippy issues

389576b

Fixed clippy issues

muraca requested changes Mar 8, 2024

View reviewed changes

improve Kitties documentation

c77120b

muraca changed the title ~~Enhanced Kitty piece with 'name' field, extended FreeKittyConstraintC…~~ TradableKitty piece Mar 8, 2024

muraca requested changes Mar 8, 2024

View reviewed changes

muraca and others added 5 commits March 8, 2024 15:08

improve TradableKitties documentation

d57f9b1

Review comments fix

db5748d

Fixed review comments from Matteo Muraca

Merge branch 'main' of https://github.com/NadigerAmit/Tuxedo

02aaf78

web service implementation

9c4f903

Web service implementation

Revert "web service implementation"

9e2c908

This reverts commit 9c4f903.

	for index in 0..transaction.outputs.len() {
	let output_ref = OutputRef {
	tx_hash,
	index: index as u32,
	};

	debug!(
	target: LOG_TARGET,
	"Checking for pre-existing output {:?}", output_ref
	);

	ensure!(
	TransparentUtxoSet::<V>::peek_utxo(&output_ref).is_none(),
	UtxoError::PreExistingOutput
	);
	}

		/// The default price of a kitten is 10.
		const DEFAULT_KITTY_PRICE: u128 = 10;

TradableKitty piece #171

Are you sure you want to change the base?

TradableKitty piece #171

Conversation

NadigerAmit commented Feb 12, 2024 • edited by muraca

coax1d left a comment

Choose a reason for hiding this comment

JoshOrndorff left a comment

Choose a reason for hiding this comment

JoshOrndorff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NadigerAmit Mar 2, 2024 • edited

Choose a reason for hiding this comment

NadigerAmit Mar 2, 2024 • edited

Choose a reason for hiding this comment

NadigerAmit Mar 2, 2024 • edited

Choose a reason for hiding this comment

NadigerAmit Mar 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muraca left a 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

NadigerAmit Mar 15, 2024 • edited

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

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

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

JoshOrndorff commented Apr 22, 2024

NadigerAmit commented Apr 27, 2024 • edited

`TradableKitty` piece #171

`TradableKitty` piece #171

NadigerAmit commented Feb 12, 2024 •

edited by muraca

NadigerAmit Mar 2, 2024 •

edited

NadigerAmit Mar 2, 2024 •

edited

NadigerAmit Mar 2, 2024 •

edited

NadigerAmit Mar 5, 2024 •

edited

NadigerAmit Mar 15, 2024 •

edited

NadigerAmit commented Apr 27, 2024 •

edited