Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[Uniques V2] Atomic NFTs swap #12285

Merged
merged 34 commits into from
Oct 5, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
3086c76
Atomic NFTs swap
jsidorenko Sep 16, 2022
12b756f
Fmt
jsidorenko Sep 16, 2022
8083eca
Merge branch 'js/uniques-v2-main-branch' into js/uniques-v2-swaps
jsidorenko Sep 19, 2022
5575d3f
Merge branch 'js/uniques-v2-main-branch' into js/uniques-v2-swaps
jsidorenko Sep 19, 2022
5564a52
Merge branch 'js/uniques-v2-main-branch' into js/uniques-v2-swaps
jsidorenko Sep 19, 2022
fc1024a
Fix benchmark
jsidorenko Sep 20, 2022
a2955be
Rename swap -> atomic_swap
jsidorenko Sep 20, 2022
bdc0538
Update target balance
jsidorenko Sep 20, 2022
a28cbcc
Rollback
jsidorenko Sep 20, 2022
504b7d5
Fix
jsidorenko Sep 20, 2022
1ccf996
".git/.scripts/bench-bot.sh" pallet dev pallet_nfts
Sep 20, 2022
ca2d1ec
Make desired item optional
jsidorenko Sep 29, 2022
78f4570
Apply suggestions
jsidorenko Sep 30, 2022
7c15814
Update frame/nfts/src/features/atomic_swap.rs
jsidorenko Sep 30, 2022
6232aa4
Rename fields
jsidorenko Sep 30, 2022
9acfb5c
Optimisation
jsidorenko Sep 30, 2022
8d6cbda
Add a comment
jsidorenko Sep 30, 2022
ee88eaa
Merge branch 'js/uniques-v2-main-branch' into js/uniques-v2-swaps
jsidorenko Sep 30, 2022
2ce1ea3
deadline -> maybe_deadline
jsidorenko Sep 30, 2022
99d1eb5
Add docs
jsidorenko Sep 30, 2022
b404705
Change comments
jsidorenko Sep 30, 2022
cccebd3
Add price direction field
jsidorenko Sep 30, 2022
2202f0d
".git/.scripts/bench-bot.sh" pallet dev pallet_nfts
Sep 30, 2022
2688fe5
Wrap price and direction
jsidorenko Oct 4, 2022
2dd02af
Fix benchmarks
jsidorenko Oct 4, 2022
e565989
Use ensure! instead of if {}
jsidorenko Oct 4, 2022
82841fc
Make duration param mandatory and limit it to MaxDeadlineDuration
jsidorenko Oct 4, 2022
7fbf339
Make the code safer
jsidorenko Oct 4, 2022
e956594
Fix clippy
jsidorenko Oct 4, 2022
b52a615
Chore
jsidorenko Oct 4, 2022
3b41ee1
Remove unused vars
jsidorenko Oct 5, 2022
239bf17
try
jsidorenko Oct 5, 2022
d086e81
try 2
jsidorenko Oct 5, 2022
c2afaa4
try 3
jsidorenko Oct 5, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions frame/nfts/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,5 +458,68 @@ benchmarks_instance_pallet! {
}.into());
}

create_swap {
let (collection, caller, _) = create_collection::<T, I>();
let (item1, ..) = mint_item::<T, I>(0);
let (item2, ..) = mint_item::<T, I>(1);
let price = ItemPrice::<T, I>::from(100u32);
let duration = T::BlockNumber::max_value();
}: _(SystemOrigin::Signed(caller.clone()), collection, item1, collection, item2, Some(price), Some(duration))
verify {
assert_last_event::<T, I>(Event::SwapCreated {
collection,
item: item1,
desired_collection: collection,
desired_item: item2,
price: Some(price),
deadline: Some(duration),
}.into());
}

cancel_swap {
let (collection, caller, _) = create_collection::<T, I>();
let (item1, ..) = mint_item::<T, I>(0);
let (item2, ..) = mint_item::<T, I>(1);
let price = ItemPrice::<T, I>::from(100u32);
let duration = T::BlockNumber::max_value();
let origin = SystemOrigin::Signed(caller.clone()).into();
Nfts::<T, I>::create_swap(origin, collection, item1, collection, item2, Some(price), Some(duration))?;
}: _(SystemOrigin::Signed(caller.clone()), collection, item1)
verify {
assert_last_event::<T, I>(Event::SwapCancelled {
collection,
item: item1,
desired_collection: collection,
desired_item: item2,
price: Some(price),
deadline: Some(duration),
}.into());
}

claim_swap {
let (collection, caller, _) = create_collection::<T, I>();
let (item1, ..) = mint_item::<T, I>(0);
let (item2, ..) = mint_item::<T, I>(1);
let price = ItemPrice::<T, I>::from(0u32);
let duration = T::BlockNumber::max_value();
let target: T::AccountId = account("target", 0, SEED);
let target_lookup = T::Lookup::unlookup(target.clone());
let origin = SystemOrigin::Signed(caller.clone());
Nfts::<T, I>::transfer(origin.clone().into(), collection, item2, target_lookup)?;
Nfts::<T, I>::create_swap(origin.clone().into(), collection, item1, collection, item2, Some(price), Some(duration))?;
}: _(SystemOrigin::Signed(target.clone()), collection, item2, collection, item1, Some(price.clone()))
verify {
assert_last_event::<T, I>(Event::SwapClaimed {
send_collection: collection,
send_item: item2,
send_item_owner: target,
receive_collection: collection,
receive_item: item1,
receive_item_owner: caller,
price: Some(price),
deadline: Some(duration),
}.into());
}

impl_benchmark_test_suite!(Nfts, crate::mock::new_test_ext(), crate::mock::Test);
}
152 changes: 152 additions & 0 deletions frame/nfts/src/features/atomic_swap.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
// This file is part of Substrate.

// Copyright (C) 2022 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

use crate::*;
use frame_support::{
pallet_prelude::*,
traits::{Currency, ExistenceRequirement::KeepAlive},
};

impl<T: Config<I>, I: 'static> Pallet<T, I> {
pub fn do_create_swap(
gilescope marked this conversation as resolved.
Show resolved Hide resolved
sender: T::AccountId,
collection_id: T::CollectionId,
item_id: T::ItemId,
desired_collection_id: T::CollectionId,
desired_item_id: T::ItemId,
jsidorenko marked this conversation as resolved.
Show resolved Hide resolved
gilescope marked this conversation as resolved.
Show resolved Hide resolved
maybe_price: Option<ItemPrice<T, I>>,
Copy link
Member

@ggwpez ggwpez Sep 21, 2022

Choose a reason for hiding this comment

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

I think the semantic of offering an NFT and a price is weird.
Now there can be no case in which i can buy an NFT just for a price.
An NFT swap should also work without price, or is this common practice?

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 the semantic of offering an NFT and a price is weird. Now there can be no case in which i can buy an NFT just for a price. An NFT swap should also work without price, or is this common practice?

The price is optional here, is it not?
It makes total sense to be able to add extra into the mix.
I'm curious though how this would work if the price needs to be added on the other end.
Looking at the logic here it seems that the price should have direction, as in who has to pay the added value - caller or the other party.

Copy link
Contributor

@ruseinov ruseinov Sep 28, 2022

Choose a reason for hiding this comment

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

The way this could be achieved is:
maybe_price
price_direction -> enum (Caller, Receiver) (names are shit, but the idea is sound, because we would never need the price specified on both ends.)

a bit off-topic: BTW I've seen some decent ideas when talking to a blockchain company a while ago. The extensions in their framework allow for sort of barter deals, what that basically means is:
we have
A - wants some cabbage, sells some rice
B - wants some rice, sells some carrots
C - wants some carrots, sells some cabbage
If the quantities match - this transaction goes through and everybody gets and sells what they want.

I know that this is not what we are doing here, but at the very least the idea of everybody using the same method call to say SELL/BUY is a good one, because it's basically a matter of direction only and then there should be a match. It's pretty much like crypto exchanges work too.

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 was thinking that to solve the order direction, a counterpart could simply register a swap :)

re your offtopic, the meta transactions could help here I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

maybe_duration: Option<<T as SystemConfig>::BlockNumber>,
) -> DispatchResult {
let item = Item::<T, I>::get(&collection_id, &item_id).ok_or(Error::<T, I>::UnknownItem)?;
ensure!(item.owner == sender, Error::<T, I>::NoPermission);

Item::<T, I>::get(&desired_collection_id, &desired_item_id)
.ok_or(Error::<T, I>::UnknownItem)?;

let now = frame_system::Pallet::<T>::block_number();
let deadline = maybe_duration.map(|d| d.saturating_add(now));
jsidorenko marked this conversation as resolved.
Show resolved Hide resolved

PendingSwapOf::<T, I>::insert(
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
&collection_id,
&item_id,
PendingSwap {
desired_collection: desired_collection_id,
desired_item: desired_item_id,
price: maybe_price,
deadline,
},
);

Self::deposit_event(Event::SwapCreated {
collection: collection_id,
item: item_id,
desired_collection: desired_collection_id,
desired_item: desired_item_id,
price: maybe_price,
deadline,
});

Ok(())
}

pub fn do_cancel_swap(
sender: T::AccountId,
collection_id: T::CollectionId,
item_id: T::ItemId,
) -> DispatchResult {
let item = Item::<T, I>::get(&collection_id, &item_id).ok_or(Error::<T, I>::UnknownItem)?;
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
let swap = PendingSwapOf::<T, I>::get(&collection_id, &item_id)
.ok_or(Error::<T, I>::UnknownSwap)?;

let is_past_deadline = if let Some(deadline) = swap.deadline {
let now = frame_system::Pallet::<T>::block_number();
now > deadline
} else {
false
};

if !is_past_deadline {
ensure!(item.owner == sender, Error::<T, I>::NoPermission);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's past the deadline and I'm not the owner is it right that I can cancel the swap (or is that not possible)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right. Anyone could cancel an outdated swap and clear the storage

Copy link
Member

Choose a reason for hiding this comment

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

But there is no incentive for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

But there is no incentive for it.

Curious how this is usually tackled. I see two options

  1. Make it worth user's while
  2. Have an on_idle job clearing outdated swaps
  3. Both?

Copy link
Contributor

Choose a reason for hiding this comment

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

At the same time this could be done as a follow-up I guess, depending on the urgency.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ruseinov I saw deposit for the storage recored in Asset pallet. The amount stored within the same record.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we either need to add deposits for everything data related, so this would incentivise users to clear the data.
But at the same time, the deposit would go back to the owner and not the method's caller. So, I don't see an incentive for the external user here to call this method.
I think this approach is mostly suitable for the on_idle job that would clear outdated data.


PendingSwapOf::<T, I>::remove(&collection_id, &item_id);

Self::deposit_event(Event::SwapCancelled {
collection: collection_id,
item: item_id,
desired_collection: swap.desired_collection,
desired_item: swap.desired_item,
price: swap.price,
deadline: swap.deadline,
});

Ok(())
}

pub fn do_claim_swap(
sender: T::AccountId,
send_collection_id: T::CollectionId,
send_item_id: T::ItemId,
receive_collection_id: T::CollectionId,
receive_item_id: T::ItemId,
maybe_receive_amount: Option<ItemPrice<T, I>>,
gilescope marked this conversation as resolved.
Show resolved Hide resolved
) -> DispatchResult {
let send_item = Item::<T, I>::get(&send_collection_id, &send_item_id)
.ok_or(Error::<T, I>::UnknownItem)?;
let receive_item = Item::<T, I>::get(&receive_collection_id, &receive_item_id)
.ok_or(Error::<T, I>::UnknownItem)?;
let swap = PendingSwapOf::<T, I>::get(&receive_collection_id, &receive_item_id)
.ok_or(Error::<T, I>::UnknownSwap)?;

ensure!(send_item.owner == sender, Error::<T, I>::NoPermission);
ensure!(swap.desired_collection == send_collection_id, Error::<T, I>::UnknownSwap);
ensure!(swap.desired_item == send_item_id, Error::<T, I>::UnknownSwap);
ensure!(swap.price == maybe_receive_amount, Error::<T, I>::UnknownSwap);

if let Some(deadline) = swap.deadline {
Copy link
Member

@ggwpez ggwpez Sep 21, 2022

Choose a reason for hiding this comment

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

I think a deadline could actually be mandatory for every swap, with respect to a MaxDeadlineDuration.
This would ensure that no ancient swaps pollute the chain, since we don't take a storage deposit. Do you think this makes sense @shawntabrizi ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The should also be means of cleaning the expired swaps up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my experience, when you make the deadline mandatory, every UI starts putting the max possible value here...
@ruseinov : yep, you can remove any expired swap offer

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest that a year seems not unreasonable. If it's not gone by then then you should have to relist it. We can always extend the max allowed swap length without upsetting people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

let now = frame_system::Pallet::<T>::block_number();
ensure!(deadline >= now, Error::<T, I>::DeadlineExpired);
jsidorenko marked this conversation as resolved.
Show resolved Hide resolved
}

if let Some(amount) = swap.price {
T::Currency::transfer(&receive_item.owner, &send_item.owner, amount, KeepAlive)?;
}

Self::do_transfer(send_collection_id, send_item_id, receive_item.owner.clone(), |_, _| {
jsidorenko marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
})?;
Self::do_transfer(
receive_collection_id,
receive_item_id,
send_item.owner.clone(),
|_, _| Ok(()),
)?;
gilescope marked this conversation as resolved.
Show resolved Hide resolved

Self::deposit_event(Event::SwapClaimed {
send_collection: send_collection_id,
send_item: send_item_id,
send_item_owner: send_item.owner,
receive_collection: receive_collection_id,
receive_item: receive_item_id,
receive_item_owner: receive_item.owner,
price: swap.price,
deadline: swap.deadline,
});

Ok(())
}
}
18 changes: 18 additions & 0 deletions frame/nfts/src/features/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// This file is part of Substrate.

// Copyright (C) 2022 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

pub mod atomic_swap;
4 changes: 4 additions & 0 deletions frame/nfts/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {

Item::<T, I>::insert(&collection, &item, &details);
ItemPriceOf::<T, I>::remove(&collection, &item);
PendingSwapOf::<T, I>::remove(&collection, &item);

Self::deposit_event(Event::Transferred {
collection,
Expand Down Expand Up @@ -124,6 +125,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
ItemMetadataOf::<T, I>::remove_prefix(&collection, None);
#[allow(deprecated)]
ItemPriceOf::<T, I>::remove_prefix(&collection, None);
#[allow(deprecated)]
PendingSwapOf::<T, I>::remove_prefix(&collection, None);
CollectionMetadataOf::<T, I>::remove(&collection);
#[allow(deprecated)]
Attribute::<T, I>::remove_prefix((&collection,), None);
Expand Down Expand Up @@ -214,6 +217,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Item::<T, I>::remove(&collection, &item);
Account::<T, I>::remove((&owner, &collection, &item));
ItemPriceOf::<T, I>::remove(&collection, &item);
PendingSwapOf::<T, I>::remove(&collection, &item);

Self::deposit_event(Event::Burned { collection, item, owner });
Ok(())
Expand Down
Loading