This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Uniques V2] Atomic NFTs swap #12285
[Uniques V2] Atomic NFTs swap #12285
Changes from 23 commits
3086c76
12b756f
8083eca
5575d3f
5564a52
fc1024a
a2955be
bdc0538
a28cbcc
504b7d5
1ccf996
ca2d1ec
78f4570
7c15814
6232aa4
9acfb5c
8d6cbda
ee88eaa
2ce1ea3
99d1eb5
b404705
cccebd3
2202f0d
2688fe5
2dd02af
e565989
82841fc
7fbf339
e956594
b52a615
3b41ee1
239bf17
d086e81
c2afaa4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use
ensure!
here for less indentation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is a match a bit clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, much cleaner indeed! Though I like the idea of @ruseinov moving those 2 fields into one struct
PriceWithDirection { amount, direction }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced with
PriceWithDirection
structThere was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious how this is usually tackled. I see two options
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering if the fields should get rolled up into a
SwapWitness
struct that holds the data about the swap that the caller is using to identify that particular swap?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm...semantically it would make sense to add such a field.
I'm thinking here how could it look like, smth like
SwapWitness{ price: PriceWithDirection {amount, direction} }
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to
witness_price
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done