Notary functions in SMC to align with phase 1.1 spec #93

terencechain · 2018-04-19T02:57:11Z

This PR is based off of #78. I've made the following SMC modifications to align with phase 1.1 spec:

References:
SMC phase 1.1 spec
Notary update sample size spec
Pyevm's SMC design (vyper)

deregister_collator and release_collator implementation

… into smc_phase1.1

…t next period

prestonvanloon · 2018-04-19T13:35:34Z

sharding/contracts/sharding_manager.sol

+  struct CollationHeader {
+    uint256 shard_id;         // pointer to shard
+    bytes32 parent_hash;      // pointer to parent header
+    bytes32 chunk_root;       // pointer to collation body


Why is it chunks root? That doesn't sound correct

Pointer? wouldn't this just be the root hash?

@rauljordan why plural?

rauljordan · 2018-04-19T03:09:41Z

sharding/contracts/sharding_manager.sol

+  mapping (address => Notary) public notary_registry;
+  mapping (address => Proposer) public proposer_registry;
+  // shard_id => (header_hash => tree root)
+  mapping (uint => mapping (bytes32 => bytes32)) public collation_trees;


can you elaborate a bit more on this mapping?

rauljordan · 2018-04-19T03:10:21Z

sharding/contracts/sharding_manager.sol

-  uint constant depositSize = 1000 ether;
+  uint constant PERIOD_LENGTH = 5;
+  // Exact collation body size (1mb)
+  uint constant COLLATION_SIZE = 2 ** 20;


Do we need this in the SMC? I don't this var think is relevant

rauljordan · 2018-04-19T03:11:43Z

sharding/contracts/sharding_manager.sol


  // Log the latest period number of the shard
-  mapping (int => int) public periodHead;
+  mapping (int => int) public period_head;


I thought the best practice in solidity was camel case? Not sure though.

Local and State Variables
Use mixedCase.

https://solidity.readthedocs.io/en/v0.3.1/style-guide.html#local-and-state-variables

rauljordan · 2018-04-19T03:12:06Z

sharding/contracts/sharding_manager.sol

-
-  // Uses a block hash as a seed to pseudorandomly select a signer from the collator pool.
-  // [TODO] Chance of being selected should be proportional to the collator's deposit.
+  event LOG(uint L);


This event is just called LOG?

It should be more descriptive, LOG is very vague. also naming the variable L is very vague again should be more descriptive

Agreed. This was from the old SMC code. I'll check with @enriquefynn on the purpose of this LOG event

It has no any usage, was using it to debug, you can remove it

rauljordan · 2018-04-19T03:13:01Z

sharding/contracts/sharding_manager.sol

-    var receiptId = numReceipts;
-    ++numReceipts;
+  // Removes the notary from the notary pool and sets deregistered period
+  function deregister_notary() public returns(bool) {


Make these funcs all camelCase plz

Functions should use mixedCase.

https://solidity.readthedocs.io/en/v0.3.1/style-guide.html#function-names

prestonvanloon · 2018-04-19T13:50:10Z

sharding/contracts/sharding_manager.sol

-  int numReceipts;
-  // Indexs of empty slots caused by the function `withdraw`
-  mapping (int => int) emptySlotsStack;
+  mapping (int => bytes32) shardead;


I mean shardHead

enriquefynn · 2018-04-19T06:34:01Z

Why those functions return true at the end? I don't think is particularly useful, since false is never returned in the function.

We should vote for camel case, like normally in solidity code or underscore, used in Vyper. Since we're not using any library, I think it's a free choice.

sharding/contracts/sharding_manager.sol

+  // mapping (int => Notary) public notaries;
+  address[] public notary_pool;
+  // proposerId => Proposer
+  // mapping (int => Proposer) public proposers;


prestonvanloon · 2018-04-19T13:36:51Z

sharding/contracts/sharding_manager.sol

+  // mapping (int => Proposer) public proposers;
+  Proposer[] public proposer_pool;
+
+  // Notary registry (deregistered is 0 for not yet deregistered notaries)


What does this mean "deregistered is 0"? If you are saying that an address will return 0 because the notary is not in the mapping then you can omit this comment because that is common sense.

prestonvanloon · 2018-04-19T13:39:33Z

sharding/contracts/sharding_manager.sol


  // Log the latest period number of the shard
-  mapping (int => int) public periodHead;
+  mapping (int => int) public period_head;


Local and State Variables
Use mixedCase.

https://solidity.readthedocs.io/en/v0.3.1/style-guide.html#local-and-state-variables

sharding/contracts/sharding_manager.sol

+    int128 period;            // Period which header should be included
+    int128 height;            // Collation's height
+    address proposer_address; // Proposer's address 
+    uint256 proposer_bid;     // Proposer's bid


prestonvanloon · 2018-04-19T13:41:32Z

sharding/contracts/sharding_manager.sol

+    // Shard id of the deposit
+    uint shardId;
+    // Deposit in ether in each shard
+    uint[] deposit;


should this be a mapping? Why is it an array? How is this supposed to work?

I think it should just be a mapping of shard_Id => deposit

sharding/contracts/sharding_manager.sol

+    require(!notary_registry[notary_address].deposited);
+    require(msg.value == NOTARY_DEPOSIT);
+
+    // update notary_sample_size


prestonvanloon · 2018-04-19T13:55:30Z

sharding/contracts/sharding_manager.sol

+    update_notary_sample_size();
+
+    uint index;
+    if (!empty_stack()) {


Can you invert this if statement so you don't have to negate the empty_stack()?

if (empty_stack()) { // do thing to empty stack } else { // do thing to non-empty stack }

prestonvanloon · 2018-04-19T13:56:22Z

sharding/contracts/sharding_manager.sol

+      index = stack_pop();
+      notary_pool[index] = notary_address;
+    }
+    else {


Formatting:
should be

} else {

prestonvanloon · 2018-04-19T13:56:59Z

sharding/contracts/sharding_manager.sol

-    // Update the latest period number
-    periodHead[_shardId] = int(_expectedPeriodNumber);
+    notary_registry[notary_address] = Notary({
+      deregistered: 0,


deregistered 0? this doesn't make much sense, wouldn't it be false?

That's period. Changed it to deregisteredPeriod

prestonvanloon · 2018-04-19T13:58:50Z

sharding/contracts/sharding_manager.sol

-    var receiptId = numReceipts;
-    ++numReceipts;
+  // Removes the notary from the notary pool and sets deregistered period
+  function deregister_notary() public returns(bool) {


Functions should use mixedCase.

https://solidity.readthedocs.io/en/v0.3.1/style-guide.html#function-names

prestonvanloon · 2018-04-19T21:17:24Z

sharding/contracts/sharding_manager.sol

+    bytes32 parentHash;       // Hash of the parent collation
+    bytes32 chunkRoot;        // Root hash of the collation body
+    uint period;              // Period which header should be included
+    uint height;              // Height of the collation


Can you remove this field? If we need it, please elaborate on what it means (the tree height since genesis)

prestonvanloon · 2018-04-19T21:18:16Z

sharding/contracts/sharding_manager.sol

+    PROPOSER_LOCKUP_LENGTH = proposer_lockup_length; 
+  }
+
+  event LOG(uint L);


Let's remove this please

prestonvanloon · 2018-04-19T21:20:26Z

sharding/contracts/sharding_manager.sol

+  event LOG(uint L);
+
+
+  function isNotaryInCommittee(uint shardId, uint period) public view returns(bool) {


Please add function comments

prestonvanloon · 2018-04-19T21:23:13Z

sharding/contracts/sharding_manager.sol

+      sampleSize = currentPeriodNotarySampleSize;
+    }
+
+    uint periodToLook = period * PERIOD_LENGTH - 1;


What is this? Looks like you convert the period to a block number then subtract one?
But the variable is called "periodToLook". Consider renaming this so it is easier to understand.

Also consider adding a comment to explain where you are looking "Check the most recent block before the period started" or something like that.

Ah! good catch! it should be block

prestonvanloon · 2018-04-19T21:25:06Z

sharding/contracts/sharding_manager.sol

+
+    uint periodToLook = period * PERIOD_LENGTH - 1;
+
+    for (uint i = 1; i <= QUORUM_SIZE; i++) {


Personal preference:
Please start with i = 0

for (uint i = 0; i < QUORUM_SIZE; i++) {

prestonvanloon · 2018-04-19T21:29:36Z

sharding/contracts/sharding_manager.sol

-  function updateGasPrice(int _receiptId, uint _txGasprice) public payable returns(bool) {
-    require(receipts[_receiptId].sender == msg.sender);
-    receipts[_receiptId].txGasprice = _txGasprice;
+  function updateNotarySampleSize() private returns(bool) {


Function comment please

prestonvanloon · 2018-04-19T21:30:35Z

sharding/contracts/sharding_manager.sol

+    require(notaryRegistry[notaryAddress].deposited);
+    require(notaryPool[index] == notaryAddress);
+
+    updateNotarySampleSize();


Why do we need to do this as part of deregistering?

prestonvanloon · 2018-04-19T21:37:44Z

sharding/contracts/sharding_manager.sol

-    TxToShard(_to, _shardId, receiptId);
-    return receiptId;
+    delete notaryRegistry[notaryAddress];
+    notaryAddress.transfer(NOTARY_DEPOSIT);


Shouldn't we use .send and check that the call was successful?
Seems like good practice, but I am not sure

prestonvanloon · 2018-04-19T21:38:56Z

sharding/contracts/sharding_manager.sol

+  uint constant LOOKAHEAD_LENGTH = 4;
+  // Number of notaries to select from notary pool for each shard in each period
+  uint constant COMMITTEE_SIZE = 135;
+  // Threshold(number of notaries in committee) for a proposal to be deem accepted


Typo remove "deem"

prestonvanloon · 2018-04-19T21:41:20Z

sharding/contracts/sharding_manager.sol

+
+  // Number of notaries
+  uint public notaryPoolLength;
+  // Indexes of empty slots caused by the function `withdraw`


Do we still need this now that the withdraw function is gone?

Still need it to track empty notary slot indices (in register/deregister function)
The comment is mislead, I will change it

prestonvanloon · 2018-04-20T00:09:29Z

sharding/contracts/sharding_manager.sol

+    // Log the latest period number of the shard
+    mapping (int => int) public periodHead;
+
+    function isNotaryInCommittee(uint shardId, uint period) public view returns(bool) {


Please add function level comments

prestonvanloon · 2018-04-20T00:12:25Z

sharding/contracts/sharding_manager.sol

+        (notaryRegistry[notaryAddress].deregisteredPeriod + NOTARY_LOCKUP_LENGTH));
+
+        delete notaryRegistry[notaryAddress];
+        notaryAddress.transfer(NOTARY_DEPOSIT);


Shouldn't we use .send and check that the call was successful?
Seems like good practice, but I am not sure

.transfer() throws on failure, it's used in most cases as it's best practice and safest way to send ether
.send() returns false on failure, it's only used in rare cases when you want to handle failure in the contract

prestonvanloon · 2018-04-20T00:14:24Z

sharding/contracts/sharding_manager.sol

+        require(!notaryRegistry[notaryAddress].deposited);
+        require(msg.value == NOTARY_DEPOSIT);
+
+        updateNotarySampleSize();


Why do we do this? Can you add a comment so that a reader can understand?

prestonvanloon · 2018-04-20T00:15:29Z

sharding/contracts/sharding_manager.sol

+        require(notaryRegistry[notaryAddress].deposited);
+        require(notaryPool[index] == notaryAddress);
+
+        updateNotarySampleSize();


Why do we do this? Can you add comment so the reader understands?

prestonvanloon · 2018-04-20T00:16:47Z

sharding/contracts/sharding_manager.sol

-            activateCollatorNum += 1;
+    function computeHeaderHash(uint256 shardId, bytes32 parentHash, 
+    bytes32 chunkRoot, uint256 period, address proposerAddress)
+    public returns(bytes32) {


Formatting.
Please format each argument on another line.

https://solidity.readthedocs.io/en/v0.3.1/style-guide.html#function-declaration

prestonvanloon · 2018-04-20T00:18:13Z

sharding/contracts/sharding_manager.sol

-}
+
+    function addHeader(uint _shardId, uint period, bytes32 chunkRoot, 
+    address proposerAddress) public returns(bool) {


Formatting.
Please format each argument on another line.

https://solidity.readthedocs.io/en/v0.3.1/style-guide.html#function-declaration

prestonvanloon · 2018-04-20T00:19:34Z

sharding/contracts/sharding_manager.sol

+        require(notaryRegistry[notaryAddress].deposited == true);
+        require(notaryRegistry[notaryAddress].deregisteredPeriod != 0);
+        require((block.number / PERIOD_LENGTH) > 
+        (notaryRegistry[notaryAddress].deregisteredPeriod + NOTARY_LOCKUP_LENGTH));


rauljordan · 2018-04-24T05:24:25Z

sharding/contracts/sharding_manager.sol

@@ -87,14 +87,9 @@ contract SMC {

      // Get the most recent block number before the period started
        uint latestBlock = period * PERIOD_LENGTH - 1;
+        uint index = uint(keccak256(block.blockhash(latestBlock), _index, shardId)) % sampleSize;


Is it possible to make this line a bit simpler? or abstract away with variables?

rauljordan · 2018-04-24T05:26:03Z

sharding/contracts/sharding_manager_test.go

+		txOpts[i].Value = notaryDeposit
+		genesis[notaryPoolAddr[i]] = core.GenesisAccount{
+			Balance: accountBalance2000Eth,
+		}


add a few comments to this section for readers to easily understand

prestonvanloon

I think we're missing a big piece of refactoring where we need to rename "collator" to "notary" . That might need to be in the next pull request to keep this one small enough to review.

prestonvanloon · 2018-04-24T16:15:54Z

sharding/collator/collator.go

+				return err
+			}
+
+			// If output is non-empty and the addr == coinbase


This comment is not accurate to the code below. Please update

Sounds good, Will fix it.

prestonvanloon · 2018-04-24T22:24:40Z

sharding/collator/collator.go

 	for s := int64(0); s < sharding.ShardCount; s++ {
 		// Checks if we are an eligible collator according to the SMC
-		addr, err := c.SMCCaller().GetEligibleCollator(&bind.CallOpts{}, big.NewInt(s), period)
+		for i := int64(0); i < sharding.NotaryCommitSize; s++ {
+			addr, err := c.SMCCaller().GetNotaryInCommittee(&bind.CallOpts{}, big.NewInt(s), big.NewInt(i))


What is going on in this method?
According to the method name and comment, this was supposed to check if we are an eligible collator, but it is checking to see if we are a notary?

It's checking if we are a notary that's in the committee for the given shard and index

My concerns about this are addressed in #96

Addressed in #96

prestonvanloon · 2018-04-24T22:26:13Z

sharding/collator/collator.go

@@ -85,11 +86,11 @@ func checkSMCForCollator(c client.Client, head *types.Header) error {
 func isAccountInCollatorPool(c client.Client) (bool, error) {
 	account := c.Account()
 	// Checks if our deposit has gone through according to the SMC
-	b, err := c.SMCCaller().IsCollatorDeposited(&bind.CallOpts{}, account.Address)
-	if !b && err != nil {
+	b, err := c.SMCCaller().NotaryRegistry(&bind.CallOpts{}, account.Address)


Again, this is checking notary registry. Are there no more collators?

No. There's no more collator. Collator has became Notary

Addressed in #96

prestonvanloon · 2018-04-24T22:30:24Z

sharding/contracts/sharding_manager.sol

+
+    /// Registers notary to notatery registry, locks in the notary deposit,
+    /// and returns true on success
+    function registerNotary() public payable returns(bool) {


Why do we return a boolean? Wouldn't the transaction fail if there was an error?
Is this standard practice for solidity?

I'm following the latest spec and py-evm's implementation: https://ethresear.ch/t/minimal-sharding-protocol-extension/1823. We can remove it if there's a strong consensus of doing so

Let's remove it if we aren't doing anything with it.

prestonvanloon · 2018-04-24T22:31:38Z

sharding/contracts/sharding_manager_test.go

+	backend := backends.NewSimulatedBackend(genesis)
+	_, _, smc, _ := deploySMCContract(backend, notaryPoolPrivKeys[0])
+
+	// Test notary 0 has not registered


What does this mean? You are testing that notary 0 has not been registered?

Yes. it's a negative test case where notary 0's not registered

I think my confusion is coming from the word "test". Can you make this say "Notary 0 has not be registered"?

prestonvanloon · 2018-04-24T22:42:54Z

sharding/contracts/sharding_manager_test.go

+
+	// Verify notary has saved the deregistered period as: current block number / period length
+	notary, _ = smc.NotaryRegistry(&bind.CallOpts{}, addr)
+	currentPeriod := big.NewInt(int64(100 / sharding.PeriodLength))


Magic number again. What is the significance of 100?

It can be any number. We want to check notary.DeregisteredPeriod has set correct. Using 100 blocks for example, the notary.DeregisteredPeriod should be set as 100/5 = 20

prestonvanloon · 2018-04-24T22:46:02Z

sharding/contracts/sharding_manager_test.go

+	}
+
+	// Fast forward 10 blocks
+	for i := 0; i < 10; i++ {


Magic number. Same comment as above.

prestonvanloon · 2018-04-24T22:47:16Z

sharding/contracts/sharding_manager_test.go

-	if depositsEventsIterator.Event.Index.Cmp(big.NewInt(0)) != 0 {
-		t.Fatalf("Collator index mismatch: %d should be 0", depositsEventsIterator.Event.Index)
+
+	balance, err := backend.BalanceAt(ctx, addr, nil)


error check?

Ok will fix.

prestonvanloon · 2018-04-24T22:48:24Z

sharding/contracts/sharding_manager_test.go

-		t.Fatalf("Collator address mismatch: %x should be %x", depositsEventsIterator.Event.Collator, addr)
+
+	if notary.Deposited {
+		t.Fatalf("Notary deposit flag should be false after released")


I think this can be t.Errorf(...). The general rule of go tests is to go as far as you can before calling Fatal. In this case, I think the test can proceed to the next part where the balance is checked. Please take a look at the test and see if there are other areas where you could call t.Errorf rather than t.Fatal because the test could still continue (although would likely fail for the other conditions). It would be nice to see many errors, if there were many.

prestonvanloon · 2018-04-24T22:50:09Z

sharding/contracts/sharding_manager_test.go

-	transactOpts.Value = collatorDeposit
-	smc.Deposit(transactOpts)
+	// Fast forward 10 blocks
+	for i := 0; i < 10; i++ {


Magic number comment. Why are you doing this?

prestonvanloon

This is wonderful. Thanks @terenc3t!

Just 1 major concern: Please remove the SMC methods return (boolean) as it serves no purpose.

prestonvanloon · 2018-04-27T23:14:51Z

sharding/collator/collator.go

@@ -85,11 +86,11 @@ func checkSMCForCollator(c client.Client, head *types.Header) error {
 func isAccountInCollatorPool(c client.Client) (bool, error) {
 	account := c.Account()
 	// Checks if our deposit has gone through according to the SMC
-	b, err := c.SMCCaller().IsCollatorDeposited(&bind.CallOpts{}, account.Address)
-	if !b && err != nil {
+	b, err := c.SMCCaller().NotaryRegistry(&bind.CallOpts{}, account.Address)


Addressed in #96

prestonvanloon · 2018-04-27T23:14:56Z

sharding/collator/collator.go

 	for s := int64(0); s < sharding.ShardCount; s++ {
 		// Checks if we are an eligible collator according to the SMC
-		addr, err := c.SMCCaller().GetEligibleCollator(&bind.CallOpts{}, big.NewInt(s), period)
+		for i := int64(0); i < sharding.NotaryCommitSize; s++ {
+			addr, err := c.SMCCaller().GetNotaryInCommittee(&bind.CallOpts{}, big.NewInt(s), big.NewInt(i))


Addressed in #96

prestonvanloon · 2018-04-27T23:16:46Z

sharding/contracts/sharding_manager_test.go

+	backend := backends.NewSimulatedBackend(genesis)
+	_, _, smc, _ := deploySMCContract(backend, notaryPoolPrivKeys[0])
+
+	// Test notary 0 has not registered


I think my confusion is coming from the word "test". Can you make this say "Notary 0 has not be registered"?

enriquefynn and others added 10 commits March 24, 2018 18:37

WIP: SMC for phase 1

841287c

Merge branch 'master' into smc_phase1

d142cff

sharding: collator functions

1401a8c

deregister_collator and release_collator implementation

sharding: tests for deregister_collator

1057d6a

sharding: get_eligible_collator with tests

825df5e

Merge branch 'smc_phase1' of https://github.com/enriquefynn/go-ethereum…

598a986

… into smc_phase1.1

sharding: /collator/noatry and align constants w/ phase 1.1 spec

8a14326

sharding: fixed up on notary's register, deregister, release functions

a207940

sharding: monitoring notary size at current period and make adjustmen…

710a24d

…t next period

sharding: function to check notary in committee

7d0eac4

terencechain changed the title ~~notary functions i SMC to align with phase 1.1 spec~~ wip: notary functions in SMC to align with phase 1.1 spec Apr 19, 2018

rauljordan reviewed Apr 19, 2018

View reviewed changes

nisdas reviewed Apr 19, 2018

View reviewed changes

prestonvanloon self-requested a review April 19, 2018 13:31

prestonvanloon requested changes Apr 19, 2018

View reviewed changes

prestonvanloon reviewed Apr 19, 2018

View reviewed changes

terencechain added 4 commits April 19, 2018 14:58

sharding: cleaned up comments in code

513119a

sharding: fixed naming styles

652a362

sharding: proposer is irrelevant for spec 1.1

b0a444e

sharding: fixed lint messages

114d34b

prestonvanloon reviewed Apr 20, 2018

View reviewed changes

terencechain added 4 commits April 19, 2018 23:41

sharding: functions descriptions

55bec2b

sharding: fixed notary tests

a1f552c

sharding: fixed notary sample function

9caa5fc

sharding: added test cases for registering notary

0b9b33f

This was referenced Apr 23, 2018

SMC Changes for a minimal sharding protocol #89

Closed

Collator design for phase 1 #79

Closed

sharding: WIP SMC for phase 1 #78

Merged

sharding: WIP collator withdraw #88

Closed

terencechain added 3 commits April 23, 2018 16:18

sharding: added notary tests for degistration and release

5b1a0e5

Merge branch 'master' into smc_phase1.1

6540bd2

sharding: added tests for sampling committees

aa22e64

terencechain changed the title ~~wip: notary functions in SMC to align with phase 1.1 spec~~ Notary functions in SMC to align with phase 1.1 spec Apr 24, 2018

rauljordan reviewed Apr 24, 2018

View reviewed changes

terencechain added 4 commits April 24, 2018 08:47

sharding: feedback changes

b45c10d

sharding: abi name changes fixed other packages

53a31d4

sharding: fixed lint

1e5315a

sharding: fixed lint

f1517cb

prestonvanloon requested changes Apr 24, 2018

View reviewed changes

sharding: latest round of feedback

0d5723d

terencechain mentioned this pull request Apr 27, 2018

Change terms Collator -> Notary to align with sharding 1.1 #96

Merged

prestonvanloon approved these changes Apr 27, 2018

View reviewed changes

sharding: fixed return bool for functions in sol

4ada125

terencechain self-assigned this Apr 27, 2018

terencechain merged commit 2922eca into prysmaticlabs:master Apr 28, 2018

terencechain deleted the smc_phase1.1 branch April 28, 2018 00:07

rauljordan added the Phase 1 label May 1, 2018

rauljordan added this to To do in Sharding Manager Contract via automation May 1, 2018

rauljordan added this to the Ruby milestone May 1, 2018

		event LOG(uint L);


		function isNotaryInCommittee(uint shardId, uint period) public view returns(bool) {


		uint periodToLook = period * PERIOD_LENGTH - 1;

		for (uint i = 1; i <= QUORUM_SIZE; i++) {

Notary functions in SMC to align with phase 1.1 spec #93

Notary functions in SMC to align with phase 1.1 spec #93

Conversation

terencechain commented Apr 19, 2018 • edited

This comment was marked as resolved.

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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enriquefynn commented Apr 19, 2018

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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

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

prestonvanloon 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

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

terencechain commented Apr 19, 2018 •

edited