Refactor SC - balance_hash, batch unlock #39

loredanacirstea · 2018-04-12T08:20:22Z

fixes raiden-network/spec#35
fixes raiden-network/spec#31

fixes #28
fixes #27
fixes #26

fixes #30

Summary

implements Smart contracts - batch unlock, settle, balance hash in balance proof spec#54
- using the balance hash in the balance proof
batch unlock happens after settleChannel because it needs to compare locksroot (only available in settleChannel) with the computer merkle_root
refactored settleChannel logic
refactored data structures

(most tests are happy cases, more will be added in a separate PR)

Gas costs

Gas costs now

TokenNetworksRegistry DEPLOYMENT 3872498
TokenNetworksRegistry.createERC20TokenNetwork 2576500
TokenNetwork DEPLOYMENT 3355362
TokenNetworksRegistry createERC20TokenNetwork 2576500

TokenNetwork.openChannel 115230
TokenNetwork.setDeposit 53475
TokenNetwork.closeChannel 93951
TokenNetwork.updateNonClosingBalanceProof 88168
TokenNetwork.settleChannel 83636  # 2 token transfers
TokenNetwork.unlock 1 locks 30716
TokenNetwork.unlock 6 locks 64605
SecretRegistry.registerSecret 45414

Initial gas costs (current master branch)

TokenNetwork.openChannel 110826
TokenNetwork.setDeposit 70862
TokenNetwork.closeChannel 106721
TokenNetwork.updateTransfer 67077   # updateNonClosingBalanceProof
TokenNetwork.settleChannel 63701  # 2 token transfers

TokenNetworkRegistry DEPLOYMENT 3224456
TokenNetwork DEPLOYMENT 2734177
TokenNetworkRegistry.createERC20TokenNetwork 2100525
SecretRegistry.registerSecret 45508

As discussed in raiden-network/spec#35

- refactored data structures - batch `unlock` after `settleChannel` - balance hash in the balance proof - refactored `settleChannel` logic raiden-network/spec#35 raiden-network/spec#31

LefterisJP · 2018-04-24T11:13:16Z

Performing a review of this PR now.

czepluch · 2018-04-24T11:23:18Z

I am also reviewing right now. Forgot to write when I started(sorry), but I think two people taking a look at this is okay.

czepluch

Looks pretty good to me. Only have some small comments, but nothing important for now. I approve, but let's wait for Lefteris' review too.

czepluch · 2018-04-24T10:09:29Z

raiden_contracts/contracts/SecretRegistry.sol


    /*
     *  Events
     */

-    event SecretRevealed(bytes32 secret);
+    event SecretRevealed(bytes32 indexed secrethash);

    function registerSecret(bytes32 secret) public returns (bool) {


Can you please add a docstring to this function?

fixed 0634caa

czepluch · 2018-04-24T10:20:59Z

raiden_contracts/contracts/TokenNetwork.sol

+        bytes32 balance_hash_or_locksroot;
+
+        // Nonce used in updateNonClosingBalanceProof to compare balance hashes during the
+        // settlement window. This is replace in `settleChannel` by the total amount of tokens


replace -> replaced

fixed 0634caa

czepluch · 2018-04-24T10:22:50Z

raiden_contracts/contracts/TokenNetwork.sol

+        uint248 settle_block_number;
+
+        // Channel state
+        // 1 = open, 2 = closed


Guess we could use an Enum here to make it more verbose, but I am fine with either.

Yes, was also thinking about this but I decided to leave it like this for now. I am planning to review the data structures again after implementing the rest of the features & writing some more tests.

LefterisJP

Big PR so lots of comments from my side

LefterisJP · 2018-04-24T11:48:05Z

raiden_contracts/contracts/TokenNetwork.sol

-        // The latest known merkle root of the pending hash-time locks, used to
-        // validate the withdrawn proofs.
-        bytes32 locksroot;
+        uint240 deposit;


why uint240 here and in the other places where deposit is used?

This was thought as an optimization for gas usage. The other two variables initialized and is_closer are packed with deposit into a single slot of 32 bytes

Are you sure this optimizes for gas succesfully and is it worth the discrepancy of using a different uint type? How much gas does this save?

opened #46
not part of this PR imo.

Agreed but this PR already makes an optimization by introducing a uint240 for deposits and a uint248 for block numbers.

And just because I agree it would make more sense to move this change, as per your issue, out of this PR.

LefterisJP · 2018-04-24T11:49:10Z

raiden_contracts/contracts/TokenNetwork.sol

+        // This is a value set to true after the channel has been closed, only if this is the
+        // participant who closed the channel.
+        // This is bytes1 and it gets packed with the rest of the struct data.
+        bool is_closer;


can you rename the variable to is_the_closer? is_closer can have a very different meaning.

fixed a2e4768

LefterisJP · 2018-04-24T11:50:42Z

raiden_contracts/contracts/TokenNetwork.sol

+        bytes32 balance_hash_or_locksroot;
+
+        // Nonce used in updateNonClosingBalanceProof to compare balance hashes during the
+        // settlement window. This is replace in `settleChannel` by the total amount of tokens


replace -> replaced

LefterisJP · 2018-04-24T11:51:25Z

raiden_contracts/contracts/TokenNetwork.sol

+        // and settling the channel.
+        // After the channel has been uncooperatively closed, this value represents the
+        // block number after which settleChannel can be called.
+        uint248 settle_block_number;


why uint248 here and in all the other places where you count block durations?

Same reason as for deposit.
settle_block_number gets packed with state into a single 32 bytes storage slot -> less gas.

Didn't we decide to use uint256 for block numbers on issue #2 to avoid packing/unpacking?

0634fa0#diff-5d5d8d0b58307a4b1624f680cbd59cbcR11 (btw, the commit message is lacking)

https://github.com/raiden-network/raiden-contracts/pull/39/files#r183699301 should fall under the same rationale IMO

@hackaugusto The case is different - in the referenced issue there was no reason to downcast and it actually increased gas usage. Here, it actually saves gas - a lot more than it consumes for type casting

okay, so I misunderstood your test on the issue then.

Just to clarify: In solidity values of a mapping are rounded up to an EVM word? Meaning that we cannot save space for primitive types, but we can save space for compound types?

(prev comment was incorrect or things changed recently)
Referencing the Warning text from http://solidity.readthedocs.io/en/v0.4.23/miscellaneous.html#layout-of-state-variables-in-storage

You can save space for primitive types storage if you have multiple small size primitive types. But they need to be defined one after another.

E.g.

bytes2 a; bytes2 b;

defined in the common contract storage will get packed together.

But mapping(bytes32 => uint64) public secret_to_block;
will save the uint64 in a storage slot defined by the mapping key -> values cannot be packed together because they can be all over the place.

I am not sure if it's worth optimizing that much for gas as we end up optimizing for the current solidity version. The way things are packed/stored can change in the future as has happened before.

How much gas do we actually save with this?

Opened #46
Imo, don't think this should be part of this PR.

LefterisJP · 2018-04-24T11:52:23Z

raiden_contracts/contracts/TokenNetwork.sol

+        uint256 channel_identifier,
+        address participant,
+        uint256 unlocked_amount,
+        uint256 returned_tokens);


closing paragraph in its own line

LefterisJP · 2018-04-24T15:21:33Z

raiden_contracts/tests/utils.py

+from functools import reduce
+from collections import namedtuple
+from web3 import Web3
+from ..utils.config import SETTLE_TIMEOUT_MIN


same thing here on relative imports as before

LefterisJP · 2018-04-24T15:26:09Z

raiden_contracts/tests/utils.py

+
+def get_packed_transfers(pending_transfers, types):
+    packed_transfers = [encode_abi(types, x[:-1]) for x in pending_transfers]
+    return reduce((lambda x, y: x + y), packed_transfers)


you could also use the aforementioned helper function here

That helper function did something else. This is (should be) just used here.

LefterisJP · 2018-04-24T15:26:54Z

raiden_contracts/tests/utils.py

+    return reduce((lambda x, y: x + y), packed_transfers)
+
+
+def get_settlement_amounts(


A small docstring explaining what this helper function returns would be helpful here

LefterisJP · 2018-04-24T15:28:48Z

raiden_contracts/utils/merkle.py

+        return MerkleTree(layers=[[EMPTY_MERKLE_ROOT]])
+
+    if not len(leaves) == len(set(leaves)):
+        raise ValueError('The items must not cointain duplicate items')


The items -> The leaves items andcointain -> contain

LefterisJP · 2018-04-24T15:31:52Z

raiden_contracts/utils/merkle.py

+
+    tree = [leaves]
+    layer = leaves
+    while len(layer) > 1:


I recognize this pattern. This is the one we were discussing in the chat in the morning and we have it in the raiden code. Can you make a note somewhere to get it abstracted it and put it in the common raiden libs and use it both in raiden and here.

It is already in raiden-libs but without the comment you added yesterday, so I opened raiden-network/raiden-libs#50
I really wanted to not have dependencies on that repo unless necessary. So, I know about it and I will look to see if I need to reuse anything else. If yes, I'll do it.

Also opened #42 to track common components with raiden-libs (which I will probably end up using)

Fixed: raiden-network#39 (comment) raiden-network#39 (comment)

Fixes raiden-network#39 (comment)

As suggested in raiden-network#39 (comment)

hackaugusto · 2018-04-24T13:07:06Z

raiden_contracts/contracts/TokenNetwork.sol

-        // the channel is open
-        uint256 settle_timeout;
-        mapping(address => Participant) participants;
+    struct BalanceData {


why not two struct definitions? the combination balance_hash locked_amonut seems invalid. Does solidity have ADTs/tagged union types?

And what about renaming it to BalanceProofData?

why not two struct definitions?

In terms of gas cost, you will end up with 10k more gas when calling settleChannel
now: 5k * 2 (2 SSTORE for modifying the 2 variables in BalanceData) = 10k
2 structs: - 10k * 2 (deleting 1 struct with balance_hash, nonce) + 20k * 2 (adding the 2nd struct) = 20k

But if the logic is not clear enough, it can be considered.

And what about renaming it to BalanceProofData?

It is BalanceProofData for balance_hash and nonce and BalanceData for locksroot and locked_amount. At least this is how we agreed in raiden-network/spec#54 .
If we combine them in 1 struct, as it is now BalanceData seems more general to me + it's shorter. But if we get another opinion saying otherwise, I'll change.

In terms of gas cost, you will end up with 10k more gas when calling settleChannel
now: 5k * 2 (2 SSTORE for modifying the 2 variables in BalanceData) = 10k
2 structs: - 10k * 2 (deleting 1 struct with balance_hash, nonce) + 20k * 2 (adding the 2nd struct) = 20k

What is adding the 2nd struct? I thought that would be the two SSTOREs.

Edit: I'm okay with the gas optimization, it's an understandable trade-off. My suggestion makes more sense if the language suports tagged unions, which would just reuse the same address space.

hackaugusto · 2018-04-24T13:19:21Z

raiden_contracts/contracts/TokenNetwork.sol

@@ -579,74 +585,112 @@ contract TokenNetwork is Utils {

    }*/

+    function updateBalanceProofData(


updateBalanceHashData or updateBalanceProofHashData are better names IMO

We are updating balance_hash and nonce = balance proof data in my opinion

updateBalanceHashData - confusing because we have balance_hash
updateBalanceProofHashData - technically, possibly more correct because signing does hashing before, but I still think updateBalanceProofData is ok and shorter. And again, I don't want confusion with balance_hash

If I say updateBalanceProofData - what does it mean to you?

If I say updateBalanceProofData - what does it mean to you?

I think about the whole balance proof transferred amount, merkleroot, nonce, etc.

And again, I don't want confusion with balance_hash

It's exactly because the function is updating the balance_hash that I gave that suggestion (and because I would call it balanceproof_hash :P ):

balance_state.balance_hash_or_locksroot = balance_hash;

~~What about updateBalanceHashAndNonce?~~ But your naming makes sense, it's mainly my problem when I think balanceproof_hash

hackaugusto · 2018-04-24T13:27:30Z

raiden_contracts/contracts/TokenNetwork.sol

            nonce,
-            transferred_amount,
-            locksroot,
            additional_hash,


shouldn't we also add the closing_signature here? (havent thought much about it, not sure what are the benefits/drawbacks)

I did not see the point in including the closing_signature here.
Not including it keeps the code more dry (1 function for all balance proof address recoveries)

Both signatures are required. The first signature proves the balance proof was sent by the channel participant and it's valid, the second proves the balance proof was validated and allows delegate calls. If the closing signature is not included this attack is possible:

The attacker makes sure to never send a balance proof with a nonce higher than the counter party's latest balance proof, and to exhaust its capacity from the channel (this is really easy to do, just wait for the counter party to use the channel twice and send a single payment that uses all the available balance).

Then the attacker closes the channel. It will provide the latest valid balance proof from the counter party to properly close it.

Then the attacker gets the counter party balance proof with a nonce equal to its latest balance proof nonce + 1, signs it, and calls updateNonClosingBalanceProof like it was a monitoring service. The balance proof and signature are valid, but for the wrong direction, and because the attacker took care to never produce a balance proof with a higher nonce the other node cant overwrite it.

The wrapping of the signatures is what proves the direction of the balance proof.

Ok, yes, agreed. Will be changed.

Fixed here 40388b3

hackaugusto · 2018-04-24T13:29:42Z

raiden_contracts/contracts/TokenNetwork.sol

-        // Make sure the second signature is from the non-closing participant
-        require(closing_requests[channel_identifier].closing_participant != non_closing_participant);
+        // Make sure the signature is from a channel participant
+        require(non_closing_participant_state.initialized);


a signature of a channel participant is not sufficient, it needs to be a signature of the non_closing party, otherwise the closer can double sign and call this with forged data.

please add require(!closing_participant_state.is_closer);

humm, well, this is enforced by require(closing_participant != non_closing_participant);, so perhaps just change the comment?

Also, have you considered keeping the invariants grouped? I think it is easier to verify them if we don't need to jump around in the contract code.

I have:

// Make sure the signatures are from different accounts require(closing_participant != non_closing_participant); [....] // Make sure the first signature is from the closing participant require(closing_participant_state.is_the_closer);

Isn't this enough?

keeping the invariants grouped

Would definitely be nice.
I try to do that whenever possible, but with require (uses REVERT opcode), the address that calls the function receives back the gas corresponding to the rest of the code after the failed require. So this means placing the require as soon as you can helps to not burn gas/eth.

IMO this should not take priority over clarity if the input is invalid, only in cases where the input is valid but a race happened (e.g. two monitoring services called update at the same time, only one of the transactions will work)

No need to change it in this PR but I agree with Augusto. Clarity should be what we aim for in smart contract code rather than providing tiny bit more gas back with REVERT

keeping track of this here #47

hackaugusto · 2018-04-24T13:36:32Z

raiden_contracts/contracts/TokenNetwork.sol

-        address participant2)
-        isClosed(channel_identifier)
-        timeoutOver(channel_identifier)
+        uint256 participant1_transferred_amount,


cant we use a compound type here? something like:

struct BalanceProof { address, uint256, uint256, bytes32 } function settleChannel(uint256, BalanceProof, BalanceProof)

I guess this would be useful only if it helps with the stack too deep error

I know. I cannot use struct yet with the current solc version that we are using. It is possible after 0.4.22, but we are waiting for it to be stabilized & the new ABI encoder changes to be introduced in web3 (neither js nor py have integrated dynamic types or structs as function arguments)

I will revisit this anyway after functionality & features are finished.

hackaugusto · 2018-04-24T14:30:50Z

raiden_contracts/contracts/TokenNetwork.sol

+        require(token.transfer(participant, unlocked_amount));
+
+        // Transfer the rest of the tokens back to the partner
+        if (returned_tokens > 0) {


could you please remove this guard or add it to the settle function too?

Yes, I noticed I did this. I will add it to the settle function.

hackaugusto · 2018-04-24T18:39:56Z

raiden_contracts/contracts/TokenNetwork.sol

+        // and settling the channel.
+        // After the channel has been uncooperatively closed, this value represents the
+        // block number after which settleChannel can be called.
+        uint248 settle_block_number;


Didn't we decide to use uint256 for block numbers on issue #2 to avoid packing/unpacking?

0634fa0#diff-5d5d8d0b58307a4b1624f680cbd59cbcR11 (btw, the commit message is lacking)

hackaugusto · 2018-04-24T18:40:22Z

raiden_contracts/contracts/TokenNetwork.sol

+        // and settling the channel.
+        // After the channel has been uncooperatively closed, this value represents the
+        // block number after which settleChannel can be called.
+        uint248 settle_block_number;


https://github.com/raiden-network/raiden-contracts/pull/39/files#r183699301 should fall under the same rationale IMO

hackaugusto · 2018-04-24T18:45:33Z

raiden_contracts/contracts/TokenNetwork.sol

    /// @notice Called on a closed channel, the function allows the non-closing participant to
    // provide the last balance proof, which modifies the closing participant's state. Can be
-    // called multiple times by anyone, as long as they provide signatures from both participants.
+    // called multiple times by anyone.


s/consisten/consistent/

reviews on comment reviews are also a thing?

hackaugusto · 2018-04-24T19:44:09Z

raiden_contracts/contracts/TokenNetwork.sol

-                el := mload(add(merkle_proof, i))
+        bytes32[] memory merkle_layer = new bytes32[](length / 96 + 1);
+
+        for (i = 32; i < length; i += 96) {


bytes is a variable length type, it's more likely the length of the merkle_tree_leaves input.

As per review raiden-network#39

As suggested in the review raiden-network#39

raiden-network/raiden#1382

fixes raiden-network#30 and raiden-network#39 (comment)

As requested in raiden-network#39 (comment) Note: these optimizations went hand in hand with other data structure decisions, this will need to be assessed in raiden-network#46

palango · 2018-04-25T13:49:22Z

raiden_contracts/contracts/TokenNetwork.sol

        added_deposit = total_deposit - participant_state.deposit;

-        // Change the state
+        // Update the participant's channel deposit
        participant_state.deposit += added_deposit;


Why not update this directly? participant_state.deposit = total_deposit ?

I was thinking of it more like a measure of security - increase with the actual amount used in the token transfer. If proper checks are in place, either way should be fine.

LefterisJP

All my comments have been addressed.

As mentioned in raiden-network#39 (comment), we need the non-closing participant to also sign the closing participant's signature, to avoid replay attacks on updateNonClosingBalanceProof.

As requested in raiden-network#39 (comment)

loredanacirstea added 2 commits April 12, 2018 09:17

Use secrethash in SecretRegistry instead of the secret

8d902cc

As discussed in raiden-network/spec#35

Use batch unlock, balance hash in balance proof

09ed693

- refactored data structures - batch `unlock` after `settleChannel` - balance hash in the balance proof - refactored `settleChannel` logic raiden-network/spec#35 raiden-network/spec#31

loredanacirstea changed the title ~~Update privacy~~ Refactor SC - balance_hash, batch unlock Apr 12, 2018

loredanacirstea added the dev: Work In Progress label Apr 12, 2018

loredanacirstea mentioned this pull request Apr 17, 2018

Smart contracts - batch unlock, settle, balance hash in balance proof raiden-network/spec#54

Merged

palango mentioned this pull request Apr 20, 2018

Update BalanceProof after contracts update raiden-network/raiden-libs#43

Closed

loredanacirstea added 3 commits April 24, 2018 01:46

Refactor data structures, balance_hash, batch unlock

a5df651

Fix tests to use new balance proof spec

d471276

Add tests for batch unlock and settleChannel

8bf702a

loredanacirstea added dev: Please Review and removed dev: Work In Progress labels Apr 24, 2018

loredanacirstea requested review from hackaugusto, LefterisJP and czepluch April 24, 2018 07:25

czepluch approved these changes Apr 24, 2018

View reviewed changes

LefterisJP suggested changes Apr 24, 2018

View reviewed changes

loredanacirstea added 3 commits April 24, 2018 20:17

Add docstrings for registerSecret

0634caa

Fixed: raiden-network#39 (comment) raiden-network#39 (comment)

TokenNetwork - rename is_closer to is_the_closer

a2e4768

Fixes raiden-network#39 (comment)

TN - separate function for computing the balance data key

2384d10

As suggested in raiden-network#39 (comment)

hackaugusto suggested changes Apr 24, 2018

View reviewed changes

TN - more code style, docstrings

0c366d0

As per review raiden-network#39

loredanacirstea mentioned this pull request Apr 24, 2018

Change closing parenthesis style for Solidity #43

Closed

loredanacirstea and others added 3 commits April 24, 2018 23:21

More code style fixes

a6fd4b5

As suggested in the review raiden-network#39

Adjust line breaks after operators

36af135

raiden-network/raiden#1382

TN - remove support for 0 value token transfers

bba73e5

fixes raiden-network#30 and raiden-network#39 (comment)

loredanacirstea mentioned this pull request Apr 25, 2018

Asses using Enum for channel states in TokenNetwork #44

Closed

Removing uint size optimizations

35665fb

As requested in raiden-network#39 (comment) Note: these optimizations went hand in hand with other data structure decisions, this will need to be assessed in raiden-network#46

loredanacirstea mentioned this pull request Apr 25, 2018

Assess keeping invariants grouped #47

Closed

palango reviewed Apr 25, 2018

View reviewed changes

LefterisJP approved these changes Apr 25, 2018

View reviewed changes

Fix updateNonClosingBalanceProof signature recovery

40388b3

As mentioned in raiden-network#39 (comment), we need the non-closing participant to also sign the closing participant's signature, to avoid replay attacks on updateNonClosingBalanceProof.

loredanacirstea mentioned this pull request Apr 25, 2018

Channel lifecycle specification #48

Closed

loredanacirstea added 2 commits April 26, 2018 02:19

Fix settleChannel token amounts calculation

5909756

As requested in raiden-network#39 (comment)

Fix travis - add rlp version constraints

7a87ae2

hackaugusto approved these changes Apr 26, 2018

View reviewed changes

loredanacirstea merged commit 95968d4 into raiden-network:master Apr 26, 2018

loredanacirstea mentioned this pull request May 14, 2018

Add proper tests for unlock and updateTransfer #18

Closed

loredanacirstea deleted the update-privacy branch July 4, 2018 10:26

loredanacirstea mentioned this pull request Jul 24, 2018

Use channel_identifier instead of the participant addresses pairs in the channel functions #193

Closed

loredanacirstea mentioned this pull request Jul 31, 2018

Architecture decision - unlocking pending transfers on-chain #202

Closed

		return reduce((lambda x, y: x + y), packed_transfers)


		def get_settlement_amounts(

		@@ -579,74 +585,112 @@ contract TokenNetwork is Utils {

		}*/

		function updateBalanceProofData(

Refactor SC - balance_hash, batch unlock #39

Refactor SC - balance_hash, batch unlock #39

Conversation

loredanacirstea commented Apr 12, 2018 • edited Loading

Summary

Gas costs

Gas costs now

Initial gas costs (current master branch)

LefterisJP commented Apr 24, 2018

czepluch commented Apr 24, 2018

czepluch 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

LefterisJP left a comment • edited Loading

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

hackaugusto Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

loredanacirstea Apr 25, 2018 • edited Loading

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

hackaugusto Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackaugusto Apr 25, 2018 • edited Loading

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

Choose a reason for hiding this comment

loredanacirstea commented Apr 12, 2018 •

edited

Loading

LefterisJP left a comment •

edited

Loading

hackaugusto Apr 24, 2018 •

edited

Loading

loredanacirstea Apr 25, 2018 •

edited

Loading

hackaugusto Apr 25, 2018 •

edited

Loading

hackaugusto Apr 25, 2018 •

edited

Loading