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

Gnosis DutchX Buyback #4

Open
wants to merge 35 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@samparsky
Copy link

samparsky commented Nov 6, 2018

Issue #3
@anxolin Can you please review

Omidiora Samuel and others added some commits Nov 6, 2018

Merge pull request #1 from anxolin/master
Example of call to the dx
added more tests
added more tests

added more tests

added more tests

added more tests

@samparsky samparsky force-pushed the samparsky:master branch from 564eb80 to 345fcfb Nov 20, 2018

@samparsky samparsky changed the title WIP: Gnosis DutchX Buyback Gnosis DutchX Buyback Nov 20, 2018

@anxolin
Copy link

anxolin left a comment

Hi @samparsky , thanks for submitting the bounty.

Yesterday I was overlooking the code, and I think it's a good start. The thing is that I would appreciate if you can document a bit the approach you are taking, because I frankly I had a bit difficulty yesterday understanding.

I'm interested in how you model the problem.
I would appreciate also if you describe the main use cases with, so anyone can understand what can you do with these contracts. You can fake a simple example for illustration if that makes it easier.

On this review, I just added some small comments on some things not related to the logic of the contract. After adding the documentation, I will do a second review of the logic itself.

Thanks for the submission. If you have any questions let me know.

Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
require(address(_sellToken) != address(0));
require(_sellToken != _buyToken);
require(_auctionIndexes.length == _auctionAmounts.length);
require(buybacks[_userAddress].sellToken == address(0)); // ensure the user address doesn't exist

This comment has been minimized.

@anxolin

anxolin Nov 28, 2018

We only allow a buy back per user? What if a user wants to do a buy back of A-WETH and B-WETH?

This comment has been minimized.

@samparsky

samparsky Nov 30, 2018

Yeah they can modify the auction indexes and amount. Since the buyback would execute in an a uction index.

This comment has been minimized.

@anxolin

anxolin Jan 7, 2019

I still don't understand the setup.

  1. I see there's a single owner, does this mean that the contract is used only for one project? Do we need to deploy a contract per owner?

  2. I don't understand the parameter _userAddress, it looks like it's not an user address, and it's just being used as an ID.

  3. It's sent a commitment for selling an amount in some auction index, but this commitment won't have any effect if someone wants to execute the postSell order, because the contract may not have enough balance of the token. You just send the amounts, but nothing forces you to add the tokens to satisfy them.

This comment has been minimized.

@samparsky

samparsky Jan 8, 2019

  1. The idea was that the contract can be used for multiple projects. It's also possible to deploy a contract per owner if needed.

  2. It's the address of the user that wants to create the buyback config. It's also used as an id, since each address is unique.

  3. Yeah, so when executing a postSellOrder the contract checks for that the user has enough balance to execute the sellOrder if not reverts the transaction. Right now nothing forces you to add the tokens to satisfy them but then when executing the postSellOrder the contract ensures you have enough balance before posting the sellOrder to the gnosis contract.

Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
Show resolved Hide resolved gnosis-buyback/test/buyback.js Outdated
Show resolved Hide resolved gnosis-buyback/test/buyback.js Outdated

@samparsky samparsky force-pushed the samparsky:master branch from 88e797f to 3121bf1 Dec 3, 2018

@samparsky

This comment has been minimized.

Copy link

samparsky commented Dec 3, 2018

@anxolin I have added more details to the readme doc & added more tests

@samparsky samparsky force-pushed the samparsky:master branch from 3121bf1 to 06efff5 Dec 3, 2018

@anxolin

This comment has been minimized.

Copy link

anxolin commented Dec 3, 2018

Thank you very much @samparsky , this week I'll try to find time to review it. Worst case I think I'll have it review by next wednesday.

@samparsky

This comment has been minimized.

Copy link

samparsky commented Dec 3, 2018

@anxolin Is there a way to possibly have it reviewed sooner?

@samparsky

This comment has been minimized.

Copy link

samparsky commented Dec 12, 2018

@anxolin Still waiting on your review :)

@anxolin

This comment has been minimized.

Copy link

anxolin commented Dec 13, 2018

I'm sorry Sam, we've been very busy this days. We'll review it as soon as we can and get back to you.

Thanks for adding the documentation. We'll review that, and we can organize a call with you after.

@samparsky

This comment has been minimized.

Copy link

samparsky commented Dec 17, 2018

@anxolin Okay cool, hopefully it doesn't turn over into next year

@samparsky

This comment has been minimized.

Copy link

samparsky commented Jan 5, 2019

@anxolin Happy new year

@anxolin
Copy link

anxolin left a comment

Happy new year @samparsky!,

First of all, sorry for the delay, Christmas is a busy season with lots of people on vacations.

Please, note that 
I receive the notifications on both gitter and Github. With the ones in Github I have enough, next time, please let's just use only Github. There's no need to SPAM other users.
Thanks for understanding.



Thanks too for adding documentation, it made the review MUCH simpler.


Anyway, I'm afraid I have some concerns regarding this contract. 
They were added as comments.

As a summary of the most important ones:




  1. Looks like the contract is meant to be used with a single owner


  2. The parameter _userAddress, base of all operations, looks like it's modelling just an ID, for identifying the buy back. Is not an user address


  3. The buy backs are not enforced. The contract allows to modify everything


  4. The buy backs are not enforced. The contract allows to have less tokens than the ones it needs to fulfil the commitment.


5. The tip is not enforced. Even if you say that you give a tip for poking the contract, you can skip the part of adding ether as a tip.

  1. You can get the Ether from tips stuck, if no-one sends a post sell order


  2. Some test don’t pass: 5 of them

  3. Some code quality notes, and visibility issue


Thanks!

@@ -0,0 +1,229 @@
# DutchX Token Buyback Implementation

This comment has been minimized.

@anxolin

anxolin Jan 7, 2019

Thanks, having the README really helps understanding the approach

"truffle": "^4.1.5"
},
"scripts": {
"test": "sh scripts/test.sh",

This comment has been minimized.

@anxolin

anxolin Jan 7, 2019

I executed the tests, and 5 were failing.

This comment has been minimized.

@samparsky

samparsky Jan 8, 2019

I will look into this

mapping (address => mapping(address => uint)) internal balances;

// mapping of user address to ether deposit value
mapping(address => uint) internal etherBalance;

This comment has been minimized.

@anxolin

anxolin Jan 7, 2019

It looks like you can get funds stuck in the contract? One example is, if you add a tip, but you don't do the post sell order when the time is right.

This comment has been minimized.

@samparsky

samparsky Jan 8, 2019

Yeah true, I would add a method to enable withdrawal of ether deposited for tips

require(address(_sellToken) != address(0));
require(_sellToken != _buyToken);
require(_auctionIndexes.length == _auctionAmounts.length);
require(buybacks[_userAddress].sellToken == address(0)); // ensure the user address doesn't exist

This comment has been minimized.

@anxolin

anxolin Jan 7, 2019

I still don't understand the setup.

  1. I see there's a single owner, does this mean that the contract is used only for one project? Do we need to deploy a contract per owner?

  2. I don't understand the parameter _userAddress, it looks like it's not an user address, and it's just being used as an ID.

  3. It's sent a commitment for selling an amount in some auction index, but this commitment won't have any effect if someone wants to execute the postSell order, because the contract may not have enough balance of the token. You just send the amounts, but nothing forces you to add the tokens to satisfy them.

* @param _auctionIndexes Auction index the to participate
* @param _auctionAmounts Auction amount to fill in auction index
*/
function modifyAuctionAmountMulti(

This comment has been minimized.

@anxolin

anxolin Jan 7, 2019

For this an all the other modification functions: Do we allow to modify a buy back?

The way I understood it was as a commitment for a project. But it looks like a project can add the buy back, and then change his mind.

This comment has been minimized.

@samparsky

samparsky Jan 8, 2019

Yeah, I figured a project can want to increase or decrease the buyback amount they want to execute based off parameters external to the contract.

Take for example Binance coin (BNB), Binance executes buyback of BNB based off the profits for the month so enabling the project to modify it accordingly seemed okay.

*/
function() external payable {
address userAddress = msg.sender;
etherBalance[userAddress] += msg.value;

This comment has been minimized.

@anxolin

anxolin Jan 7, 2019

3 things related to this method:

  • Is there a strategy to avoid losing funds in the contract? How do I get them out after we end our commitment?
  • How do we know for which tip is which ether?
  • How do we ensure that we have enough ether to pay the tips? You can add a tip, and not the ether, right?

This comment has been minimized.

@samparsky

samparsky Jan 10, 2019

The tipping implementation is a soft contract, that is if the user has enough ether for tipping the person poking the postSellOrder function is tipped else nothing happens.

Also each project address is mapping to their ether deposit values.

Show resolved Hide resolved gnosis-buyback/contracts/CoolAppdependencies.sol Outdated
Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol
Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
Show resolved Hide resolved gnosis-buyback/contracts/Buyback.sol Outdated
@samparsky

This comment has been minimized.

Copy link

samparsky commented Jan 10, 2019

@anxolin Thanks for the review. I have gone through the code and made a number of changes to the logic. The postSellOrder function now ensures the user has enough balance to post a sellOrder.
Also, I have removed _userAddress from all the function signatures as it's not needed.

Also added a function withdrawEther to enable withdrawing ether deposit from the contract.

I have added error messages to require where needed.

The test cases have been fixed and is working okay now.

Please let me know when you will be available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment