Origin: Prevent seller from purchasing own listing

[micahalcorn]

In the past, we have experienced some unexpected behavior with reviews when a seller and a buyer are the same account. The decision was recently made to prevent DApp users from purchasing their own listings. There may be some instances where a user wants to cause a listing to no longer be available, but this should be accomplished by "closing" the listing. A user with a large inventory of listing might prefer to show some as sold, perhaps as a result of selling them off-chain, but I'm not convinced that allowing a seller to be a buyer of the same listing is the appropriate way to handle this case. Unless there is an argument for allowing it, let's prevent this behavior at the contract level.

[cuongdo]

@DanielVF I'd like to take a look at this, unless you have something in-progress for this

[cuongdo]

The change to the smart contract is straightforward and works in my local branch. However, some of the JS tests have the seller purchase their own listing, which is violating the new safeguard I added. For example, this code buys the listing using the same web3 that was used to create the listing: https://github.com/OriginProtocol/origin-js/blob/master/test/resource_purchases.test.js#L58

Some options I'm thinking of are:

1. Creating a buyer's instance of web3, which will use a different account to make the purchase.
2. Add a setCurrentAcount() function to src/contract-service.js so that the tests can easily change the current account to the buyer account.

Option 1 seems a little cleaner, because it involves less fiddling with web3's internal state.

Any thoughts? Is there something easier and/or cleaner I'm missing @micahalcorn @wanderingstan @DanielVF?

[DanielVF]

I'd be concerned that option 1 would make the tests look uglier, and option 2 might get things confused if the account was left set to a different account after a test. I'd prefer option 1 out of the two.

Another option might be a contractService.asAccount method, used like:

contractService.asAccount(buyer, async ()=> {
    await purchases.buyerConfirmReceipt(purchase.address, { rating: 3 })
}

This would change the account while the code is run, then switch it back again. Would just need to catch, change back, and then rethrow in case of errors.

[tyleryasaka]

Closed by https://github.com/OriginProtocol/origin-js/pull/223

[micahalcorn]

This is no longer enforced in the new marketplace contract. 🤕

[micahalcorn]

It was decided in today's sprint meeting that this validation would be moved to the JavaScript layer.

[tyleryasaka]

Some thoughts:

1. Preventing seller from purchasing their own listing at the origin-js layer is not very useful. It doesn't help from a user experience perspective because we're already not showing the purchase button for user's own listings at the Dapp level. And it's not helpful from a security perspective because someone could just bypass origin-js and make a direct call to the smart contracts.

2. We could, instead, flag offers with the same buyer and seller as invalid in origin-js and filter them out when returning offers for a listing. I'm not sure what the point of this would be though. If it's meant to be some sort of security feature, it's not very useful at that. New Ethereum accounts can be created in seconds. If someone wanted to manipulate the system by buying their own listings, this simple validation would not stop them. If it's meant to prevent weird states in the Dapp, I'd be curious for someone to elaborate what problems this causes. If the Dapp is architected properly, I wouldn't expect there to be any bugs that arise from the buyer and seller being the same. If there are bugs, I would fix them at the Dapp level.

Bottom line, I think this validation can give the illusion of security while not offering any, or can give the illusion of offering UX value when not really adding any of that either. In the end I think this just adds unnecessary complexity. I don't think we should be concerned about this right now.

[micahalcorn]

Closing this for now based on the comments above 😐