search

GnosisEcosystemFund / Gnosis-Bounties-

2 stars 3 forks source link

Gnosis DutchX Buyback #4

Open samparsky opened 5 years ago

samparsky commented 5 years ago

Issue #3 @anxolin Can you please review

samparsky commented 5 years ago

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

anxolin commented 5 years ago

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 commented 5 years ago

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

samparsky commented 5 years ago

@anxolin Still waiting on your review :)

anxolin commented 5 years ago

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 commented 5 years ago

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

samparsky commented 5 years ago

@anxolin Happy new year

samparsky commented 5 years ago

@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

anxolin commented 5 years ago

Hi @samparsky ,

I still feel the issues raised in the last review weren't addressed.

You can reply to them point by point if you want.

  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


samparsky commented 5 years ago

  1. The contract can be used by single or multiple owners

  2. The parameter _userAddress has been removed.

  3. Yeah, the buybacks allow to modify parameters to a buyback. It doesn't mean they aren't enforced. e.g. A project can decide to change params of a montly buyback due to factors like profit or crypto bear market. But then I can remove the modify functions if you aren't convinced.

  4. You don't have to have the deposited tokens to add a buyback. A scenario is when a project does monthly buybacks. But when invoking postSellOrder it ensures the project has enough token deposit to fulfill the buyback. https://github.com/GnosisEcosystemFund/Gnosis-Bounties-/blob/638ac2447f9778a961fca6774b9c6b305f1ab7f4/gnosis-buyback/contracts/Buyback.sol#L442

  5. Yes the tip is not enforced its optional, a project can decide to tip or not. If they decide to tip it's enforced if not then it's not enforced

  6. You can withdraw the Ether for the tips through the withdrawEther function https://github.com/GnosisEcosystemFund/Gnosis-Bounties-/blob/638ac2447f9778a961fca6774b9c6b305f1ab7f4/gnosis-buyback/contracts/Buyback.sol#L593

  7. All the test cases pass you might need to update your local branch

  8. A number of code changes have been made

samparsky commented 5 years ago

@anxolin hello, have you been able to check it out?

anxolin commented 5 years ago

From your answers I get that it doesn't create the commitment.

The commitment is required, because the whole point of this contract is to offer the guarantees to the users that you will buy back in a given auction. You can make a commitment bigger (more stake), but in any case you shouldn't be able, by design, to fail to fulfill it.

samparsky commented 5 years ago

@anxolin Is it possible to have a 5 mins proper discussion on Gitter on the needed changes and the requirements of the application. This one month back and forth isn't productive and its quite a waste of effort and time.

I understand that you are busy but it would be awesome if we can schedule 5 mins next week Monday to just discuss next steps and appropriate system design

anxolin commented 5 years ago

Hi Sam, I agree, but I think the requirements are described https://gitcoin.co/issue/GnosisEcosystemFund/Gnosis-Bounties-/3/1626, and I already commented the issues with the current approach you are taking.

The biggest fundamental problem, was commented already in the first review. The contract should allow to pre-commit the participation in specific auctions. Note that the current contract, doesn't create any commitment that can be enforced, it's just a register of the intention of doing so, and I don't see how this offer any guarantees to the users that want this buy back to happen.

I'm afraid I cannot give you the details on how to do it, nor I have capacity right now to do so. I'm happy to answer concrete questions or a proposal though.

I think you've done already a nice job, I'm positive you can make the changes to make it work

samparsky commented 5 years ago

@anxolin The changes required have been made. Please kindly review

samparsky commented 5 years ago

@anxolin Hello its been quite a while

anxolin commented 5 years ago

I'm sorry @samparsky we are a bit out of capacity, but I'll promise I'll find time for you this week.

Looking forward to see your solution already!

anxolin commented 5 years ago

Hei Sam!

I see that you modified the contract in the right direction, but I still have some concerns about the contract related to what I was saying in the previous reviews.

I'll write my thoughts. Let's see it with some examples:

  1. I can create a buy back for an auction that will be next week, people are prepared to participate in that auction because they saw my commitment, BUT, right before the auction, I can decide to remove my buy back, so I end up not participating. Basically, I broke my promise.

  2. I don't see how the tip is enforced. For example, I say to my users, if you submit my buy back, I'll give you 0.1 Ether, but I don't have 0.1 Ether in my account. Furthermore, it would be a easy way for me to break my commitment of participating in the auction. This is because the check is done in postSellOrder. So not only I don't pay the Ether to the user submitting the transaction to make me participate, I'll won't participate in the auction I committed to, and on top of that, I'll make this user to loose gas because of the revert of the transaction.

  3. I'm not 100% sure what happens when I don't participate in the auction, and no-one enforces me to participate. Can I get the money back of the contract?

  4. It feels that I can still change the buy backs as I please. For example I commit to burning the tokens, but then I change my mind and I just call modifyBurn, or instead of burning in the address 0x0 I decide to usemodifyBurnAddress to send the tokens to my account (again, breaking my commitment). I feel this limitation is because of what I already commented in all the previous reviews. This contract allows only one buyback setup per user (address), this is why I think you are letting the user to change anything, because in other case, the user will have to use another account for doing another buyback with another setup. Using an ID, instead of the address, and allowing to create any number of immutable buy backs for a user, feels for natural in my opinion.

@samparsky let me know if I'm mistaken on my previous assumptions

samparsky commented 5 years ago

@anxolin Your comments are right, I am almost done with making the changes you suggested.

For number 3, yeah you should be able to get your money back after maybe a set time defined by the user or a defined time by the contract e.g. 3 months

Would that be okay?

samparsky commented 5 years ago

@anxolin I am done making the changes regarding your comments

  1. its not possible to break your promise anymore as all buybacks are immutable
  2. The tip is enforced, its required before creating the buyback
  3. A user is allowed to releaseFunds of a buyback if it expires. The minimum expiry time is one month. So a user can call releaseBuybackFunds(buybackid) after the buyback expires to have access to the funds
  4. It's no longer possible to modify buybacks as you can create multiple buybacks.

Also the contract has been upgraded to 0.5.2 and the dx-contracts to the latest version. Looking forward to your comment.

samparsky commented 5 years ago

@anxolin I have made changes to the effect of your comments and have responded to your review above.

Each buyback is a single buyback now which addresses a number of issues raised above i.e.

{
  ...
  _auctionIndex: 35
auctionAmount: 20
}
samparsky commented 5 years ago

Hello @anxolin, I am hoping you can review the new changes asap

samparsky commented 5 years ago

@anxolin Hello

samparsky commented 5 years ago

@anxolin Welcome to the new month :)

samparsky commented 5 years ago

@anxolin Hello

samparsky commented 5 years ago

@anxolin Hello