Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decentraland + Gnosis Safe: Create test cases for a failed flow #273

Closed
marcelomorgado opened this issue Mar 16, 2019 · 4 comments · Fixed by #274 or #369
Closed

Decentraland + Gnosis Safe: Create test cases for a failed flow #273

marcelomorgado opened this issue Mar 16, 2019 · 4 comments · Fixed by #274 or #369
Labels
docs Something should be documented better
Milestone

Comments

@marcelomorgado
Copy link
Contributor

That flow isn't working because of the highlighted situation:

I) Pre conditions:

  1. Gnosis safe wallet is funded with enough mana to buy some parcels/estates.
  2. Gnosis safe wallet is funded with enough ethers to transfer to ephemeral account.

II) On boarding:

  1. Ephemeral account will be funded with some ethers to pay for gas (from Gnosis safe wallet).
  2. Gnosis safe approves ephemeral account to spend their mana tokens.
  3. Ephemeral account approves Marketplace to spend their mana tokens (actually tokens from Gnosis Safe). Not sure if this allowance-of-allowance will work.

III) Buy operation:

  1. Ephemeral account executes an open order from Marketplace
  2. The asset bought will go to the ephemeral account balance.

A test case should be created to cover this case.

The step II.3 doesn't work. The marketplace executes erc20.transferFrom(msg.sender, ...) so the ephemeral account should have balance.

MANAToken

// msg.sender here is the marketplace contract

 function transferFrom(address _from, address _to, uint256 _value) returns (bool) {
   var _allowance = allowed[_from][msg.sender];

   // Check is not needed because sub(_allowance, _value) will already throw if this condition is not met
   // require (_value <= _allowance);

   balances[_to] = balances[_to].add(_value);
   balances[_from] = balances[_from].sub(_value);
   allowed[_from][msg.sender] = _allowance.sub(_value);
   Transfer(_from, _to, _value);
   return true;
 }
@marcelomorgado marcelomorgado added the docs Something should be documented better label Mar 16, 2019
@pcowgill
Copy link
Member

pcowgill commented Mar 18, 2019

@marcelomorgado I wonder if we should create an issue on Decentraland's marketplace-contracts repo to create a version of _executeOrder that doesn't hardcode the from account as msg.sender

https://github.com/decentraland/marketplace-contracts/blob/3b044b12db28a4201e066a21beb97a88c6f449bf/contracts/marketplace/Marketplace.sol#L343

@marcelomorgado
Copy link
Contributor Author

@marcelomorgado I wonder if we should create an issue on Decentraland's marketplace-contracts repo to create a version of _executeOrder that doesn't hardcode the from account as msg.sender

https://github.com/decentraland/marketplace-contracts/blob/3b044b12db28a4201e066a21beb97a88c6f449bf/contracts/marketplace/Marketplace.sol#L343

Yes, It'll be nice.

@marcelomorgado
Copy link
Contributor Author

marcelomorgado commented Mar 18, 2019

Remaining task. Make asserts instead of skip failed test cases.
Refs: #274 (comment)

@marcelomorgado
Copy link
Contributor Author

marcelomorgado commented Mar 18, 2019

I'm reopening that because of the latest comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Something should be documented better
Projects
None yet
2 participants