Marketplace contract does not assert if one is an admin for an asset

[geofmureithi]

Parent issue: https://github.com/eigerco/nebula/issues/81

The code in reference: https://github.com/eigerco/nebula/blob/e431bb39e56e66062cd2db24775d041b0b6fc479/contracts/marketplace/src/lib.rs#L78-L81

[eloylp]

I was studying this case, and i think i understand it better now.

The current contract only changes the owner of the sold asset on its internal state. When we try to do the change in the stellar network by including the following code at the end of the buy_listing method:

let asset_client = StellarAssetClient::new(&env, &asset);
asset_client.set_admin(&buyer);

It fails because we would need both auths (seller.require_auth()) and buyer (buyer.require_auth()) in the buy_listing method invokation in order to:

• Change the ownership of the asset (seller's auth needed).
• Transfer the money to the seller (buyer's auth needed).

This is a multi party auth problem, where we need the authorization (signatures) of both the buyer and the seller to make the deal in the same transaction invokation.

I am looking for more information regarding this, if you have ideas, they are welcome :)

cc @geofmureithi @mariopil

[eloylp]

I think another way of approaching the Marketplace contract would be to allow the contract to be the owner of the listed assets. That way, we would also avoid a possible owner change executed by another different contract, which would corrupt the internal state of our Marketplace contract.

So the contract would change to:

• The seller can put an asset on sale, by transfering the ownership of the asset to the contract address in the moment of the listing.
• The seller can recover the ownership of a listed asset anytime, by calling a recover() method, while the asset is still for sale. That would remove the asset from the listing.
• In the moment of the buy operation, the contract will transfer the funds from the buyer (as it already has its auth) to the seller. Since the asset already belongs to the contract, the seller auth is not needed for transfering the ownership to the buyer.

For me the above looks a good approach. Let me know your thoughts @geofmureithi @mariopil

[mariopil]

I was thinking about the same, but have you tried it? Whenever I want to set an admin of the asset it panics (event if I want to set the seller as admin).

[geofmureithi]

Unfortunately, I also tried changing ownership to the contract itself but it was also unsuccessful.

[geofmureithi]

Change the ownership of the asset (seller's auth needed).

I don't think the seller auth is needed when buy listing is called. Seller auth should be checked during creating or updating a listing. From the design, only the seller has the ability to list something so if its listed, the seller should just get their tokens and a event emitted.

[eloylp]

but have you tried it?

Yes, by adding this lines to the current buy_listing() method:

let asset_client = StellarAssetClient::new(&env, &asset);
asset_client.set_admin(&buyer);

Part of the error i am getting is:

4: [Failed Diagnostic Event (not emitted)] contract:c2ab29be18acce66e8dd665475d233741de11fb019dbb1f064d69389524056e2, topics:[error, Error(Auth, InvalidAction)], data:["contract call failed", set_admin, [Address(Contract(f0ace8305b14216b7f92bc022aa54e4bdfc97137ab78e989d703a988b19b20ec))]]
5: [Failed Diagnostic Event (not emitted)] contract:c9ce1109884c91dd1756cb5a930f7edd05792f42e84552741ace0d3757f69c03, topics:[error, Error(Auth, InvalidAction)], data:["[recording authorization only] encountered authorization not tied to the root contract invocation for an address. Use `require_auth()` in the top invocation or enable non-root authorization.", Address(Contract(1a8c291cde9b2b82caf99455f4bfff9708880dacedb84fc53ba76585c4a6ccfd))]

So that confirms suspects that i f we want to change the ownership of the asset that belongs to seller , we need at least its authorization. Under my eyes this makes complete sense or anyone else could change the asset ownership.

Also, the internal state (regarding ownership) of our contract is a parallel state regarding the blockchain. Which means another contract different to ours, could perform an owner change, corrupting the internal, now outdated state of our contract, thus making it fail in subsequent invokations.

Because of the above :point_up: i think the simplest way is to transfer asset ownership to the contract in the moment of the listing, so any other contract cant change its ownership but us. This also should solve the need to seller auth, when we call set_admin() in the moment of the buy , as contract would be already the owner of the asset.

I think i am going to try performing commented modifications just to double check things on my side and try to solve any other issue.

[eloylp]

Created https://github.com/eigerco/nebula/pull/96 guys, moving to review ! :fast_forward:

[eloylp]

Alright ! I think its time to write some updates and doubts here, as we were commenting in the pr, we need to change the integration tests for this to work, as integration tests needs to recreate the testing environment in a testnet blockchain, thus the mocks strategy is not used here.

Here are some conceptual things i think i am finding:

First of all, an existencial question, what kind of token assets should this Marketplace contract be able to trade with ? I am currently discerning among 2 types of tokens:

• Tokens that are used for providing liquidity, representing fungible value. Here we would be just trading with admin rights.
• NFT or non fungible tokens, that can represent art, a document, etc... This tokens contracts normally have a balance of 1 stroop, the indivisible tenth million part of an XLM.

I believe trading with one or the another would require different set of operations in the marketplace contract in order to perform an ownership change:

• In case of a token that represents a coin with a balance, probably what we are doing calling set_admin() is good enough ? AFAIK, the admin would be the one that can manage the balance of that token among others.

• In case of an NFT, things looks different. NFT contracts will hold a very limited amount if not only one stroop of balance. After the process of issuing the NFT takes place, an ownership change here would be basically represented for that single balance of one stroop, to be transfered to the new owners account. Thats it, the NFT now belongs to another account.

So we have 2 different strategies here for changing the "ownership" of an asset, depending of the kind of asset we want to trade. Or, should this marketplace be only about NFTs ? - thus doing a payment transfer we would be done.

Some helpful resources:

• https://developers.stellar.org/docs/issuing-assets/how-to-issue-an-asset
• https://www.reddit.com/r/Stellar/comments/m9mklm/stepbystep_creating_an_nft_on_stellar_network/
• https://medium.com/stellar-community/best-practices-for-creating-nfts-on-stellar-5c91e53e9eb9

Let me know your thoughts @geofmureithi @mariopil

[mariopil]

I don't have opinion on this one, would leave the decision up to you.

[eloylp]

Ok, i am moving to implement only the NFT option.

[eloylp]

@geofmureithi , what was the motivation to require the old asset price in order to update it ? Some kind of optimistic lock ? :thinking:

[geofmureithi]

The idea is to ensure the seller is selling the right asset

[eloylp]

The idea is to ensure the seller is selling the right asset

I think now we need to do it in another way. Allow me to explain. Actually, this is the code executed for creating a listing:

let mut assets: Map<Address, Asset> = storage.get(&DataKey::Assets).unwrap();
        assets.set(
            asset.clone(),
            Asset {
                owner: seller.clone(),
                price,
                listed: true,
            },
        );

We are using an Address as identifier not only of the asset, but also as the identifier of the offer. But i think that are 2 different things. Thinking in NFTs, an NFT asset contract can have a liquidity > 1 . That means we could have 2 different accounts trying to sell the same NFT but in different offers. With the current code, it cant happen. The second offer would overwrite the previous entry.

My suggestion here would be to change the contract. A call to create_listing(...) should return an auto-inc id , that would the unique identifier of an offer and should be used for further interactions with it (like removing, pausing, etc..). That way, callers only need to remember an id to operate.

Unless you have another ideas, i am going to change the contract trying to follow this comment ideas.

[eloylp]

What makes more sense ?

A: "As a seller i can put an offer for an (asset, quantity) and buyers can buy only the entire offer, including all quantities"

or

B: "As a seller i can put an offer for an (asset, quantity) and buyers can buy the assets individually"

For me, A is the winner. As the seller can sell one asset of "A" for (i.e) 100 XLM if bought alone, or for 50 XLM if more are bought.

Thoughts ? @geofmureithi @mariopil

[eloylp]

What makes more sense ?

A: "As a seller i can put an offer for an (asset, quantity) and buyers can buy only the entire offer, including all quantities"

or

B: "As a seller i can put an offer for an (asset, quantity) and buyers can buy the assets individually"

For me, A is the winner. As the seller can sell one asset of "A" for (i.e) 100 XLM if bought alone, or for 50 XLM if more are bought.

Thoughts ? @geofmureithi @mariopil

Timeout ! Went for A

[eloylp]

Solved an insightful problem with community here:

https://discordapp.com/channels/897514728459468821/1169987533011173467/1169987533011173467

For the record.

[eloylp]

Hello @geofmureithi and @mariopil

Looks like we have a fully working contract now ! :partying_face: . I updated the PR #96 description with the major changes. This is ready for final review ! :next_track_button: