Feature/crowdsale refactor #744

fiiiu · 2018-02-15T13:33:03Z

Fixes #739

🚀 Description

Refactor of crowdsale models. Still based on the inheritance architecture used in OZ (a model based on composition is still to expensive in terms of gas). Base Crowdsale contract is made as abstract as possible but functional nonetheless. Simple extension classes add features and customized behavior. Intended as a foundation to be expanded, where many more features can be implemented in future pull requests.

📘 I've reviewed the OpenZeppelin Contributor Guidelines
✅ I've added tests where applicable to test my new functionality.
📖 I've made sure that my contracts are well-documented.
🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

theethernaut · 2018-02-15T14:33:33Z

contracts/mocks/WhitelistedCrowdsaleImpl.sol

+    ERC20 _token
+  ) public
+    Crowdsale(_rate, _wallet, _token)
+    //Ownable() //Do I need to call it anyway?! --apparently not


Base constructors only need to be referenced when you need to pass parameters to them, so no, Ownable is usually not needed to be referenced.
Can we remove the comment here?

Sorry, left this behind, removed.

theethernaut · 2018-02-15T14:38:47Z

questions.md

@@ -0,0 +1,135 @@
+# Questions


I'd remove this file from the PR.
These kind of TODO files are more a personal thing and there is no need to keep it in the repo's history for everyone to see.

Yup, sorry, residual file, didn't mean to add it in the first place, removed.

theethernaut · 2018-02-15T14:47:57Z

test/crowdsale/PremintedCrowdsale.test.js

@@ -0,0 +1,67 @@
+import ether from '../helpers/ether';


Not sure about the name "PremintedCrowdsale" here...
In a way, the base Crowdsale is also preminted, because it assumes that it has a positive balance for the associated token. In this case, we also assume that the token has been pre-minted, AND assume that the crowdsale has approval/allowance.

We had disscussed this with @facuspagnuolo, since ApprovedCrowdsale wasn't too explanatory.. But I fully agree with your comment that the base is also preminted. Any other proposal?

Some ideas:
DelegateCrowdsale, ProxyCrowdsale, ForwardingCrowdsale, AuthorizedCrowdsale?

AllowanceCrowdsale?

theethernaut · 2018-02-15T14:53:17Z

contracts/crowdsale/emission/PremintedCrowdsale.sol

+  }
+
+  // TODO: consider querying approval left and end crowdsale if depleted
+  // But approval could be increased..


See https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/token/ERC20/StandardToken.sol#L59

I'm not sure I understand this, my point in that reply was that maybe the wallet is depleted, but then the allowance could be increased by increaseApproval() and the crowdsale continue, right? Do we not want to allow this?

The function I have in mind simply returns the current allowance.

function remainingTokens() public returns (uint256) { return token.allowance(_tokenWallet, this); }

Great, I'll put that in.

theethernaut · 2018-02-15T18:02:09Z

contracts/crowdsale/validation/TimedCrowdsale.sol

+
+  function _preValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {
+    super._preValidatePurchase(_beneficiary, _weiAmount);
+    require(now >= startTime && now <= endTime);


Consider using
require(!hasExpired());

You mean substitute
require(now >= startTime && now <= endTime);
for:
require(now >= startTime);
require(!hasExpired());
?
We still need to check we are after startTime, so I'm not sure what the advantage of this would be...

Agreed. My bad =P

I would abstract that require into another inTimeRange modifier, to make the code easier to read and because it may come handy when working with a TimedCrowdsale. Thoughts?

maybe withinBuyingPeriod

theethernaut · 2018-02-15T18:04:11Z

contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol

+
+  function _postValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {
+    super._postValidatePurchase(_beneficiary, _weiAmount);
+    contributions[_beneficiary] = contributions[_beneficiary].add(_weiAmount);


I don't like the fact that we are not only validating here, but also updating the user's contribution.
Perhaps we should add a new abstract method on the base Crowdsale to do this sort of thing?

True, but the user contribution is only tracked for validation purposes.. We would need something like a separate _updateValidationState().. Maybe too much, since we already have _pre and _post validation schemes.

I agree that this new state member is only tracked for the sake of validation.
I also don't like _updateValidationState(), since as you say, seems a little to bloated.
Now, consider a crowdsale extension that simply keeps track of additional user data and allows users to query it individually or statistically. This function processPurchase() would be the entry point and the extension would not be a validation in itself. However, IndividuallyCappedCrowdsale as a validation would use updatePurchase().

Done. Please check whether we want it to be like this, as Crowdsale is a bit more bloated now..

fiiiu

test review

theethernaut · 2018-02-15T18:21:43Z

contracts/crowdsale/price/IncreasingPriceCrowdsale.sol

+    uint256 elapsedTime = now - startTime;
+    uint256 timeRange = endTime - startTime;
+    uint256 rateRange = initialRate - finalRate;
+    return initialRate.sub(elapsedTime.mul(rateRange).div(timeRange));


Consider using
rateRange = finalRate - initialRate for consistency and easier readability. (.sub is replaced by .add)

That would make rateRange negative, we require(_initialRate >= _finalRate); in order to work with unsigned ints (hence IncreasingPrice). We could add a DecreasingPriceCrowdsale, but I'm skeptical it would be of much use.

Ah yes, slope of the rate curve is negative. I was thinking about the slope of the price curve.
My bad again then.

I'm with @ajsantander in here, this is confusing. We should leave a clarification to avoid silly millionaire mistakes.

…to solve Crowdsale selection in tests, better way?

…acing hasEnded())

…rited from Crowdsale

…med, mocks changed. Capped tests passing

…emoved unused dependencies in tests

…ed IndividuallyCappedCrowdsaleImpl.sol and corresponding tests, passed.

…dsales. checked that it IS indeed inherited, but leaving it there as per Frans suggestion.

facuspagnuolo

LGTM! some minor comments

facuspagnuolo · 2018-02-19T21:28:44Z

contracts/crowdsale/validation/TimedCrowdsale.sol

+  modifier inTimeRange {
+    require(now >= startTime && now <= endTime);
+    _;
+  }


nit, but we may want to place modifiers after the state variables :)

whoops, I took ordering from solidity docs, will fix.

Actually, it is between constructor and other methods in https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/ownership/Ownable.sol for instance..

Which should it be, before or after constructor?

facuspagnuolo · 2018-02-19T21:30:54Z

contracts/crowdsale/validation/WhitelistedCrowdsale.sol

+  modifier isWhitelisted(address _beneficiary) {
+    require(whitelist[_beneficiary]);
+    _;
+  }


facuspagnuolo · 2018-02-19T21:31:14Z

contracts/crowdsale/validation/WhitelistedCrowdsale.sol

+   * @param _beneficiaries Addresses to be added to the whitelist
+   */
+  function addCrowdToWhitelist(address[] _beneficiaries) external onlyOwner {
+    for (uint i = 0; i < _beneficiaries.length; i++) {


we can use uin256 explicitly

facuspagnuolo · 2018-02-19T21:31:20Z

contracts/crowdsale/validation/UserCappedCrowdsale.sol

+   * @param _cap Wei limit for individual contribution
+   */
+  function setGroupCap(address[] _beneficiaries, uint256 _cap) external onlyOwner {
+    for (uint i = 0; i < _beneficiaries.length; i++) {


we can use uin256 explicitly

facuspagnuolo · 2018-02-19T21:32:15Z

contracts/mocks/CappedCrowdsaleImpl.sol

@@ -1,20 +1,19 @@
 pragma solidity ^0.4.18;

-
-import "../crowdsale/CappedCrowdsale.sol";
+//import "../examples/SimpleToken.sol";


can we remove this line?

facuspagnuolo · 2018-02-19T21:32:53Z

contracts/mocks/RefundableCrowdsaleImpl.sol

-
+import "../token/ERC20/MintableToken.sol";
+import "../crowdsale/distribution/RefundableCrowdsale.sol";
+import "../crowdsale/distribution/utils/RefundVault.sol";


do we need this one?

facuspagnuolo · 2018-02-19T21:33:38Z

contracts/mocks/UserCappedCrowdsale.sol

@@ -0,0 +1,19 @@
+pragma solidity ^0.4.18;
+
+//import "../examples/SimpleToken.sol";


facuspagnuolo · 2018-02-19T21:33:49Z

contracts/mocks/WhitelistedCrowdsaleImpl.sol

@@ -0,0 +1,19 @@
+pragma solidity ^0.4.18;
+
+//import "../examples/SimpleToken.sol";


shrugs · 2018-02-20T01:17:04Z

contracts/crowdsale/Crowdsale.sol

- * minted as contributions arrive, note that the crowdsale contract
- * must be owner of the token in order to be able to mint it.
+ * @dev Crowdsale is a base contract for managing a token crowdsale,
+ * allowing investors to purchase tokens with ether. This contract implements


I've traditionally seen multiline @dev comments either prefixed with @dev all the way down

/** * @dev like * @dev this */

The docs don't specify what happens for multiline, though, so I'm not sure what the standard is. Off the top of my head, though, I think OZ primarily uses the @dev prefix per-line, but I could be wrong and I'm not particularly partial

Hi @shrugs. Throughout the OZ codebase, a single @dev at the beginning of a multi-sentence description seems to be the norm (examples: Bounty.sol, DayLimit.sol). Also, the automatically-generated docs parse NatSpec following this convention.

The NatSpec specification is not at all clear about how to handle this case, but there are two examples within the document which use a muti-line @notice tag (link).

thanks for the heads up!

shrugs · 2018-02-20T03:09:40Z

contracts/crowdsale/Crowdsale.sol

+   * @param _beneficiary Address performing the token purchase
+   * @param _weiAmount Value in wei involved in the purchase
+   */
+  function _preValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {


I'm down to do that, now that we're making sweeping changes. Obviously updated across the file(s) where appropriate

shrugs · 2018-02-20T03:10:14Z

contracts/crowdsale/Crowdsale.sol

+   * @param _beneficiary Address performing the token purchase
+   * @param _weiAmount Value in wei involved in the purchase
+   */
+  function _postValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {


what are some example use-cases for this, especially since we don't use it in these contracts? If the answer is just "why not allow it" that's fine too

For example, if you are performing a purchase that involves a call to another contract, you can check that the call worked the way you were expecting here

Ah, gotcha, for invariants and such. Nice.

shrugs · 2018-02-20T03:13:20Z

contracts/crowdsale/validation/TimedCrowdsale.sol

+   */
+  function TimedCrowdsale(uint256 _startTime, uint256 _endTime) public {
+    require(_startTime >= now);
+    require(_endTime >= _startTime);


nitpick: should endTime just be > _startTime? having a crowdsale open for exactly one second in time doesn't seem useful.

(could also ignore this comment because it's unnecessarily restrictive)

shrugs · 2018-02-20T03:13:52Z

contracts/crowdsale/validation/TimedCrowdsale.sol

+   * @dev Checks whether the period in which the crowdsale is open has already elapsed.
+   * @return Whether crowdsale period has elapsed
+   */
+  function hasExpired() public view returns (bool) {


any reason we're not using the existing naming of hasEnded? I don't mind either way, but it's good to know why we changed the api.

hasEnded was the prior method that encompassed ending either by reaching endTime or by reaching the cap in a CappedCrowdsale. Now the purchase validation is done in separate places so we need to split this (hasExpired and capReached). While we could call this one hasEnded, a crowdsale could also end "prematurely" by reaching its cap, thus, be ended but not expired..

shrugs · 2018-02-20T03:23:21Z

contracts/crowdsale/price/IncreasingPriceCrowdsale.sol

+
+  /**
+   * @dev Constructor, takes intial and final rates of tokens received per wei contributed.
+   * @param _initialRate Number of tokens a buyer gets per wei at the start of the crowdsale


Do we have to enforce that the linear function here must make the tokens increase in price? I expect a crowdsale that gets cheaper over time could be desired as well. I'm not sure there's a benefit to requiring an increase. We should let the creator make a linear function of their own slope. (and in that case this contract could be LinearlyScaledRateCrowdsale

The point of restricting the price to be increasing was to be able to use unsigned ints throughout.. Plus I didn't see much use for a decreasing price crowdsale, but we could add this more general crowdsale flavor later if you think it might be useful!

No worries, I'm happy with that logic 👍

shrugs · 2018-02-20T03:24:56Z

contracts/crowdsale/validation/WhitelistedCrowdsale.sol

@@ -0,0 +1,58 @@
+pragma solidity ^ 0.4.18;


Excited for this contract, people keep asking for it :D

shrugs · 2018-02-20T03:26:08Z

contracts/examples/SampleCrowdsale.sol

    CappedCrowdsale(_cap)
    FinalizableCrowdsale()
    RefundableCrowdsale(_goal)
-    Crowdsale(_startTime, _endTime, _rate, _wallet, _token)
+    TimedCrowdsale(_startTime, _endTime)
+    Crowdsale(_rate, _wallet, _token)


I notice that we should probably be following the "most basic class first" rules here, like we do in the other *Impl.sol files. Is there a reason Crowdsale() is last? or should it be the first, since it's the most basic class?

Actually, it's perhaps that this might not actually work, since the Crowdsale is inherited from last and doesn't call super.*

shrugs · 2018-02-20T03:26:55Z

contracts/mocks/FinalizableCrowdsaleImpl.sol

@@ -13,7 +13,8 @@ contract FinalizableCrowdsaleImpl is FinalizableCrowdsale {
    address _wallet,
    MintableToken _token
  ) public
-    Crowdsale(_startTime, _endTime, _rate, _wallet, _token)
+    TimedCrowdsale(_startTime, _endTime)


likewise here for "most base-like"

shrugs · 2018-02-20T03:32:14Z

contracts/mocks/IncreasingPriceCrowdsaleImpl.sol

+    ERC20 _token,
+    uint256 _initialRate,
+    uint256 _finalRate
+  ) public


can we follow the sytnax we use across the project for this, here and across this PR? It would look like:

function IncreasingPriceCrowdsaleImpl ( uint256 _startTime, uint256 _endTime, address _wallet, ERC20 _token, uint256 _initialRate, uint256 _finalRate ) public Crowdsale(_initialRate, _wallet, _token) TimedCrowdsale(_startTime, _endTime) IncreasingPriceCrowdsale(_initialRate, _finalRate) { }

(where the visibility modifier is on an equal indentation with the rest of the modifiers)

…rowdsale

facuspagnuolo

LGTM!

maraoz

Great PR! Please see minor corrections on naming pls

maraoz · 2018-02-20T19:22:34Z

contracts/crowdsale/Crowdsale.sol

+   * @param _beneficiary Address performing the token purchase
+   * @param _weiAmount Value in wei involved in the purchase
+   */
+  function _preValidatePurchase(address _beneficiary, uint256 _weiAmount) internal {


maraoz · 2018-02-20T19:28:36Z

contracts/crowdsale/emission/AllowanceCrowdsale.sol

+   * @param _beneficiary Token purchaser
+   * @param _tokenAmount Amount of tokens purchased
+   */
+  function _emitTokens(address _beneficiary, uint256 _tokenAmount) internal {


I'm not super happy with this name

discussed offline with @fiiiu and @facuspagnuolo and decided to go with _deliverTokens 🍕

maraoz · 2018-02-20T19:30:22Z

contracts/crowdsale/validation/UserCappedCrowdsale.sol

+ * @title UserCappedCrowdsale
+ * @dev Crowdsale with per-user caps.
+ */
+contract UserCappedCrowdsale is Crowdsale, Ownable {


Maybe we can think of a better name?

decided offline with @facuspagnuolo and @fiiiu to go with IndividuallyCappedCrowdsale

maraoz · 2018-02-20T19:32:52Z

contracts/crowdsale/validation/WhitelistedCrowdsale.sol

+   * @dev Adds list of addresses to whitelist. Not overloaded due to limitations with truffle testing. 
+   * @param _beneficiaries Addresses to be added to the whitelist
+   */
+  function addCrowdToWhitelist(address[] _beneficiaries) external onlyOwner {


rename to addManyToWhitelist pls

(discussed offline)

* Basic idea * Fine tuning idea * Add comments / tidy up Crowdsale base class * fixed TimedCrowdsale constructor * added simple crowdsale test * added HODL directory under home to store unused contracts. ugly hack to solve Crowdsale selection in tests, better way? * Capped no longer inherits from Timed, added capReached() method (replacing hasEnded()) * added SafeMath in TimedCrowdsale for safety, CHECK whether it is inherited from Crowdsale * several fixes related to separating Capped from Timed. functions renamed, mocks changed. Capped tests passing * added TimedCrowdsaleImpl.sol, TimedCrowdsale tests, passed * added Whitelisted implementation and test, passed. * removed unnecessary super constructor call in WhitelistedCrowdsale, removed unused dependencies in tests * renamed UserCappedCrowdsale to IndividuallyCappedCrowdsale, implemented IndividuallyCappedCrowdsaleImpl.sol and corresponding tests, passed. * homogeneized use of using SafeMath for uint256 across validation crowdsales. checked that it IS indeed inherited, but leaving it there as per Frans suggestion. * adding questions.md where I track questions, bugs and progress * modified VariablePriceCrowdsale, added Impl. * finished VariablePrice, fixed sign, added test, passing. * changed VariablePrice to IncreasingPrice, added corresponding require() * MintedCrowdsale done, mock implemented, test passing * PremintedCrowdsale done, mocks, tests passing * checked FinalizableCrowdsale * PostDeliveryCrowdsale done, mock, tests passing. * RefundableCrowdsale done. Detached Vault. modified mock and test, passing * renamed crowdsale-refactor to crowdsale in contracts and test * deleted HODL old contracts * polished variable names in tests * fixed typos and removed comments in tests * Renamed 'crowdsale-refactor' to 'crowdsale' in all imports * Fix minor param naming issues in Crowdsale functions and added documentation to Crowdsale.sol * Added documentation to Crowdsale extensions * removed residual comments and progress tracking files * added docs for validation crowdsales * Made user promises in PostDeliveryCrowdsale public so that users can query their promised token balance. * added docs for distribution crowdsales * renamed PremintedCrowdsale to AllowanceCrowdsale * added allowance check function and corresponding test. fixed filename in AllowanceCrowdsale mock. * spilt Crowdsale _postValidatePurchase in _postValidatePurchase and _updatePurchasingState. changed IndividuallyCappedCrowdsale accordingly. * polished tests for linter, salve Travis * polished IncreasingPriceCrowdsale.sol for linter. * renamed and polished for linter WhitelistedCrowdsale test. * fixed indentation in IncreasingPriceCrowdsaleImpl.sol for linter * fixed ignoring token.mint return value in MintedCrowdsale.sol * expanded docs throughout, fixed minor issues * extended test coverage for IndividuallyCappedCrowdsale * Extended WhitelistedCrwodsale test coverage * roll back decoupling of RefundVault in RefundableCrowdsale * moved cap exceedance checks in Capped and IndividuallyCapped crowdsales to _preValidatePurchase to save gas * revert name change, IndividuallyCapped to UserCapped * extended docs. * added crowd whitelisting with tests * added group capping, plus tests * added modifiers in TimedCrowdsale and WhitelistedCrowdsale * polished tests for linter * moved check of whitelisted to modifier, mainly for testing coverage * fixed minor ordering/polishingafter review * modified TimedCrowdsale modifier/constructor ordering * unchanged truffle-config.js * changed indentation of visibility modifier in mocks * changed naming of modifier and function to use Open/Closed for TimedCrowdsale * changed ordering of constructor calls in SampleCrowdsale * changed startTime and endTime to openingTime and closingTime throughout * fixed exceeding line lenght for linter * renamed _emitTokens to _deliverTokens * renamed addCrowdToWhitelist to addManyToWhitelist * renamed UserCappedCrowdsale to IndividuallyCappedCrowdsale

fiiiu added the next label Feb 15, 2018

theethernaut reviewed Feb 15, 2018

View reviewed changes

fiiiu commented Feb 15, 2018

View reviewed changes

theethernaut reviewed Feb 15, 2018

View reviewed changes

theethernaut and others added 21 commits February 15, 2018 15:59

Basic idea

49b16cf

Fine tuning idea

fc7af3c

Add comments / tidy up Crowdsale base class

d54f799

fixed TimedCrowdsale constructor

f48c150

added simple crowdsale test

3f5680b

added HODL directory under home to store unused contracts. ugly hack …

a8a14df

…to solve Crowdsale selection in tests, better way?

Capped no longer inherits from Timed, added capReached() method (repl…

469a999

…acing hasEnded())

added SafeMath in TimedCrowdsale for safety, CHECK whether it is inhe…

90f0973

…rited from Crowdsale

several fixes related to separating Capped from Timed. functions rena…

3ffe518

…med, mocks changed. Capped tests passing

added TimedCrowdsaleImpl.sol, TimedCrowdsale tests, passed

a5aaf94

added Whitelisted implementation and test, passed.

8a3cfb9

removed unnecessary super constructor call in WhitelistedCrowdsale, r…

6343246

…emoved unused dependencies in tests

renamed UserCappedCrowdsale to IndividuallyCappedCrowdsale, implement…

2c22337

…ed IndividuallyCappedCrowdsaleImpl.sol and corresponding tests, passed.

homogeneized use of using SafeMath for uint256 across validation crow…

8c8fed1

…dsales. checked that it IS indeed inherited, but leaving it there as per Frans suggestion.

adding questions.md where I track questions, bugs and progress

962b5bc

modified VariablePriceCrowdsale, added Impl.

bbb2dfa

finished VariablePrice, fixed sign, added test, passing.

3fdb6da

changed VariablePrice to IncreasingPrice, added corresponding require()

53ec3cc

MintedCrowdsale done, mock implemented, test passing

2be5806

PremintedCrowdsale done, mocks, tests passing

b814a05

checked FinalizableCrowdsale

dd05925

fiiiu added 4 commits February 19, 2018 16:39

added group capping, plus tests

a70e3ed

added modifiers in TimedCrowdsale and WhitelistedCrowdsale

5f4fc83

polished tests for linter

47ce79f

moved check of whitelisted to modifier, mainly for testing coverage

5dab495

facuspagnuolo reviewed Feb 19, 2018

View reviewed changes

fiiiu added 2 commits February 19, 2018 18:45

fixed minor ordering/polishingafter review

dd338f1

modified TimedCrowdsale modifier/constructor ordering

8376baa

shrugs reviewed Feb 20, 2018

View reviewed changes

fiiiu added 7 commits February 20, 2018 11:24

unchanged truffle-config.js

8ad6a10

changed indentation of visibility modifier in mocks

107fdc4

changed naming of modifier and function to use Open/Closed for TimedC…

530d85d

…rowdsale

changed ordering of constructor calls in SampleCrowdsale

9fe61b8

changed startTime and endTime to openingTime and closingTime throughout

de8e88f

fixed exceeding line lenght for linter

6c38f46

Merge branch 'master' into feature/crowdsale-refactor

33839c1

facuspagnuolo previously approved these changes Feb 20, 2018

View reviewed changes

maraoz suggested changes Feb 20, 2018

View reviewed changes

fiiiu added 3 commits February 20, 2018 17:02

renamed _emitTokens to _deliverTokens

df03099

renamed addCrowdToWhitelist to addManyToWhitelist

c6def0e

renamed UserCappedCrowdsale to IndividuallyCappedCrowdsale

c177f01

fiiiu dismissed facuspagnuolo’s stale review via c177f01 February 20, 2018 20:02

maraoz approved these changes Feb 20, 2018

View reviewed changes

maraoz merged commit c05918c into OpenZeppelin:master Feb 20, 2018

maraoz removed next labels Feb 20, 2018

4000D mentioned this pull request May 11, 2018

priority between purchase_limit in <stages> and <valid_purchase> tokamak-network/tokyo#19

Closed

		@@ -0,0 +1,19 @@
		pragma solidity ^0.4.18;

		//import "../examples/SimpleToken.sol";

Feature/crowdsale refactor #744

Feature/crowdsale refactor #744

Conversation

fiiiu commented Feb 15, 2018 • edited

🚀 Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiiiu left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrugs Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrugs Feb 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo left a comment

Choose a reason for hiding this comment

maraoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fiiiu commented Feb 15, 2018 •

edited

fiiiu left a comment •

edited

shrugs Feb 20, 2018 •

edited

shrugs Feb 20, 2018 •

edited