Refactoring of TxQueue and Manager #530

dshulyak · 2018-01-03T15:29:24Z

Based on PR: #527
Fixes: #338

Changes to TxQueue:

factored out into a separate module to ensure that only exported methods will be used
deleted EnqueuAsync as it is not used, it is always possible to restore it from git, if it
will be ever needed
notifications moved to a manager, with the goal of simplifying txqueue and separate concerns
between manager and txqueue
simplified API for processing transactions
After analyzing code I removed all redundant methods and currently all the processing can be done with Get and Done methods.
added inprogress map to store transactions that are taken for processing
and removed Inprogress variable from transaction, the goal is to cleanup tx structure
removed Discard channel from transaction, transaction.Err can be used to understand
if transaction was discarded or failed due to another error
transient errors stored as a global variable of queue module for simplicity, it is unlikely
that they will become dynamic
covered queue with unit tests

Changes to Manager:

simplified code that manages notifications, all handlers were removed and notifiers
refactored to be simple functions that are called in several places
notifications can be turned off by calling DisableNotifications on manager
made a function out of CreateTransaction method, as it can be used as a simple utility

dshulyak · 2018-01-04T13:22:27Z

@adambabik this change is ready for review, please take a look when you will have time. it is based on previous pr, so all of the relevant changes are in a88f9eb

divan · 2018-01-04T13:29:06Z

@dshulyak thanks for PR. I'm going to review it today or tomorrow. The only thing that immediately brings attention is the size of the PR. Large PRs take more times to review. If there is a chance to split it into a few smaller PRs that would be great. If not, please wait until at least 2 approves.

dshulyak · 2018-01-04T13:35:50Z

@divan this PR is based on #527. So, the only commit that needs to be reviewed here is the last one (atm a88f9eb). Stats for this commit +431 −533. I changed some module names and i had to regenerate some mocks, so half of it should be trivial to review.

dshulyak · 2018-01-04T13:43:12Z

Tests failed with An error occurred while generating the build script.

dshulyak · 2018-01-04T18:37:32Z

Added one more commit on top of previous that fixes remaining data races in transactions module 39d03d8

dshulyak · 2018-01-05T08:43:38Z

e2e/transactions/transactions_test.go

-
-	log.Info(fmt.Sprintf("Number of transactions queued: %d. Queue size (shouldn't be more than %d): %d",
-		i, txqueue.DefaultTxQueueCap, txQueue.Count()))
+	time.Sleep(2 * time.Second)


comment is lost, restore

dshulyak · 2018-01-05T08:44:57Z

geth/common/types.go

+	Context context.Context
+	Args    SendTxArgs
+	Done    chan struct{}
+	Discard chan struct{}


remove discard

dshulyak · 2018-01-05T08:45:20Z

geth/common/types.go

@@ -272,6 +247,9 @@ type TxQueueManager interface {

 	// DiscardTransactions discards given multiple transactions from transaction queue
 	DiscardTransactions(ids []QueuedTxID) map[QueuedTxID]RawDiscardTransactionResult
+
+	// Disable notifictions to a device
+	DisableNotificactions()


safe to remove from interface, as it is used only in tests

dshulyak · 2018-01-05T08:46:03Z

geth/transactions/queue/queue.go

+	transactions map[common.QueuedTxID]*common.QueuedTx
+	inprogress   map[common.QueuedTxID]empty
+
+	// TODO don't use another goroutine for eviction


dshulyak · 2018-01-05T08:48:21Z

geth/transactions/txqueue_manager_test.go

@@ -112,6 +114,7 @@ func (s *TxQueueTestSuite) TestCompleteTransaction() {
 	s.NoError(tx.Err)
 	// Transaction should be already removed from the queue.
 	s.False(txQueueManager.TransactionQueue().Has(tx.ID))
+	<-w


after rebase it will be changed to a wait with timeout

dshulyak · 2018-01-05T08:48:28Z

geth/transactions/txqueue_manager_test.go

@@ -264,4 +268,5 @@ func (s *TxQueueTestSuite) TestDiscardTransaction() {
 	s.Equal(queue.ErrQueuedTxDiscarded, tx.Err)
 	// Transaction should be already removed from the queue.
 	s.False(txQueueManager.TransactionQueue().Has(tx.ID))
+	<-w


dshulyak · 2018-01-05T08:48:44Z

geth/transactions/queue/queue.go

@@ -193,6 +193,7 @@ func (q *TxQueue) Done(id common.QueuedTxID, hash gethcommon.Hash, err error) er

 func (q *TxQueue) done(tx *common.QueuedTx, hash gethcommon.Hash, err error) {
 	delete(q.inprogress, tx.ID)
+	tx.Err = err


remove L201

dshulyak · 2018-01-05T08:51:08Z

geth/transactions/txqueue_manager.go


+func (m *Manager) validateAccount(tx *common.QueuedTx) (*common.SelectedExtKey, error) {


move verifyAccountPassword password to validateAccount

dshulyak · 2018-01-05T08:55:33Z

geth/transactions/txqueue_manager.go

 	log.Info("complete transaction", "id", queuedTx.ID)
-	var emptyHash gethcommon.Hash
 	log.Info("verifying account password for transaction", "id", queuedTx.ID)
 	config, err := m.nodeManager.NodeConfig()


do we to nofity on node config error? if not move it to CompleteTransaction

dshulyak · 2018-01-05T09:13:58Z

there are 2 potential follow-ups for this refactoring that can simplify txqueue even more.

evictionLoop can be refactored as a pre-check when a transaction is enqueued. it would be simple operation on a slice, and won't have any significant overhead. it might even slightly improve performance
QueuedTx can be made immutable. currently, the lifecycle of tx can be described as:

created with args and id
updated with err, hash and event to done channel in 3 cases
- tx completed
- tx timed out
- tx discarded

However, consumers of tx are interested only in the hash and err. Atm there are 2 consumers:

caller of completeTransaction - will receive hash and err as a return arguments
caller of waitTransaction - can receive hash and err as an event to a channel, and channel can be stored as part of queue or manager and can be given to a consumer as, txqueue.Subscribe(tx.ID)

Let me know if this makes sense to you

adambabik · 2018-01-07T21:51:21Z

Thanks @dshulyak for this huge effort! You rock! I will review it in details tomorrow.

Regarding the follow-ups:

I think evictionLoop can be removed completely. I have no idea what it is for and why we need to store so many transactions. We could simply stop accepting more than N and return an error. Or evict only when a new tx is enqueued without the background go-routine.
@rasom @oskarth, do you guys remember what was the purpose of eviction? Is it related to security, performance or something else? I don't like this idea because we discard transactions without informing the user about it.
Ideally, I would love to encapsulate dealing with transaction objects in one structure. The only thing that would be passed around would be a transaction ID. This way we can make sure that the transaction object is not accessed concurrently and the whole logic is nicely encapsulated and isolated.
I like the subscription idea. I guess it would work like this:

// wait for the transaction to finish
done := txqueue.Subscribe(tx.ID)
<-done

// get details, hash or error
hash, err := txqueue.TransactionHash(tx.ID)
// or only error
err := txqueue.TransactionErr(tx.ID)

dshulyak · 2018-01-08T12:32:31Z

ci issue #539

adambabik

I added some change requests. It looks very promising 👏 However, I must also say it's hard to review because of the number of changes and the previous PR included (and I know there is no better way :().

Also, I still need to figure out if we can remove eviction completely.

I will ping the rest of the team to review #527 so that we can merge it first and have this PR smaller.

adambabik · 2018-01-08T17:11:51Z

geth/transactions/queue/queue.go

@@ -36,33 +33,38 @@ var (
 	ErrInvalidCompleteTxSender = errors.New("transaction can only be completed by the same account which created it")
 )

+// remove from queue on any error (except for transient ones) and propagate
+var transientErrs = map[string]bool{


Is there any reason why this map is not map[error]bool?

it is for errors returned from an eth client, jsonError{Message: "error"}

Ah ok. Errors from EthClient are dynamically created so even if an error has the same message, it will be a different object, thus relying on strings. Make sense 👍 Also, cool that error messages are derived from packages so everything should be fine.

I would suggest adding a comment explaining this structure as looking at its initialization it's tempting to change it to map[error]bool. Or a unit test that will test this property.

adambabik · 2018-01-08T17:12:21Z

geth/transactions/queue/queue.go

+	account.ErrNoAccountSelected.Error(): true, // account not selected
+}
+
+type empty struct{}


I am not sure if this is needed.

it was a common practice in some opensource projects to use such type for such cases, but I can avoid using it

Let's leave it then. Maybe it's just me and everyone are ok with it.

adambabik · 2018-01-08T17:14:58Z

geth/transactions/queue/queue.go

-	if q.txEnqueueHandler == nil { //discard, until handler is provided
-		log.Info("there is no txEnqueueHandler")
-		return nil
+	if (tx.Hash != gethcommon.Hash{}) {


What if a transaction errored? Before it would not have Hash set but Error.

It looks like this method is not thread-safe:

tx.Hash access is not guarded,

It's possible to enqueue already enqueued transaction (it looks like this was an issue before as well).

i thought that it makes sense to enqueu transaction again, even if an error was not transient.
but to be fair, is it even required? i dont think that i've seen a code path that can actually try to enqueue same transaction again

just for the reference, in this commit d75f6f0 (PR #537), i removed this condition completely and using a channel to return error and a hash.

but i am relying just on the tests, and i might be wrong if there is some history behind it

Have you thought about some particular use cases?

In our case, if a transaction reports an error, and it's not transient, then the transaction is removed and a signal is sent. If the error is transient, the transaction stays in the queue and a signal is sent. I don't recall any use cases that would allow or require putting the same transaction again to the queue, thus, I would throw an error in case of such an attempt.

i will add error comparison for now, and will get back to this topic in #537

adambabik · 2018-01-08T17:21:47Z

geth/transactions/queue/queue.go

+		if _, inprogress := q.inprogress[id]; inprogress {
+			return tx, ErrQueuedTxInProgress
+		}
+		q.inprogress[id] = empty{}


Why do we set the transaction to inprogress map here in Get method? Getters should not perform any side effects.

I noticed that in the current code there is only one place where Get is called (CompleteTransaction), so I wanted to get rid of the methods that are not used

or better to say redundant

yeah, it doesn't make sense i will add separate method for Get and another for marking transaction as inprogress

dshulyak · 2018-01-08T18:01:01Z

@adambabik you can also review particular commit by selecting it from a github changelog. I can't post a link because github changes it to a wrong one.

dshulyak · 2018-01-22T09:02:18Z

@divan @adambabik i rebased this pr and fixed comments after initial review

adambabik

Thanks for rebasing, it's really easy to review it now.

adambabik · 2018-01-22T15:46:42Z

geth/common/utils.go

+
+// CreateTransaction returns a transaction object.
+func CreateTransaction(ctx context.Context, args SendTxArgs) *QueuedTx {
+	return &QueuedTx{


Maybe we can also change a name of this struct to Transaction? Or eventually TransactionQueued if the first one is too verbose.

idk, name seems fine to me

adambabik · 2018-01-22T15:47:13Z

geth/common/utils.go

+// CreateTransaction returns a transaction object.
+func CreateTransaction(ctx context.Context, args SendTxArgs) *QueuedTx {
+	return &QueuedTx{
+		ID:      QueuedTxID(uuid.New()),


Once we also discussed getting rid of QueuedTxID and just using a string type. What do you think about it?

i tried and it somewhat increased the size of this pr (because of mocks). i would prefer to create a separate pr which would be trivial to review

adambabik · 2018-01-22T15:47:45Z

geth/common/utils.go

+		Hash:    common.Hash{},
+		Context: ctx,
+		Args:    args,
+		Done:    make(chan struct{}, 1),


Can it be a non-buffered channel?

i think that original idea was not to block the producer (complete transaction method). i would keep it this way, any reason not to?

adambabik · 2018-01-22T15:50:29Z

geth/transactions/notifications.go

+	// EventTransactionFailed is triggered when send transaction request fails
+	EventTransactionFailed = "transaction.failed"
+
+	SendTransactionNoErrorCode        = "0"


Shouldn't error codes be a number? Maybe something like:

const ( SendTransactionDefaultErrorCode int = iota + 1 SendTransactionPasswordErrorCode ... )

good idea, will try to change this

adambabik · 2018-01-22T15:51:46Z

geth/transactions/notifications.go

+type ReturnSendTransactionEvent struct {
+	ID           string            `json:"id"`
+	Args         common.SendTxArgs `json:"args"`
+	MessageID    string            `json:"message_id"`


What is MessageID?

i see that it is derived from context, but context is actually never updated

Oh, now I remember what it is. Let's leave it and we will create a follow-up issue to research with Clojure Team if we can get rid of this.

adambabik · 2018-01-22T15:53:28Z

geth/transactions/notifications.go

+
+// NotifyOnReturn returns handler that processes responses from internal tx manager
+func NotifyOnReturn(queuedTx *common.QueuedTx) {
+	if queuedTx.Err == nil {


Please add a comment why transactions without error are skipped (because we don't need to send any signals, but it may not be obvious).

adambabik · 2018-01-22T15:56:13Z

geth/transactions/queue/queue.go

 }

 // NewTransactionQueue make new transaction queue
-func NewTransactionQueue() *TxQueue {
+func NewQueue() *TxQueue {


It can also be func New() as queue is the name of the package.

adambabik · 2018-01-22T15:57:52Z

geth/transactions/queue/queue.go

-	if q.txEnqueueHandler == nil { //discard, until handler is provided
-		log.Info("there is no txEnqueueHandler")
-		return nil
+	if (tx.Hash != gethcommon.Hash{} || tx.Err != nil) {


This logic can be encapsulated as common.QueuedTx's method, e.g. tx.IsProcessed().

let's get back to it in this PR #537

adambabik · 2018-01-22T17:20:58Z

geth/transactions/queue/queue.go

+}
+
+// LockInprogress returns transcation and locks it as inprogress
+func (q *TxQueue) LockInprogress(id common.QueuedTxID) (*common.QueuedTx, error) {


Do we need to return common.QueuedTx?

i wanted to avoid doing get and then lock, particularly i was thinking about SELECT FOR UPDATE sql statement when i added this method

maybe naming is unfortunate, though

I can't find where it is used except for a test file.

If we have LockInProgress() and Get() and call it in this order everything should be ok without race conditions, right?

maybe naming is unfortunate, though

Agree. I had to go to the function definition to understand what does it mean. Maybe GetAndLock?

@adambabik it's used in (*Manager).CompleteTransaction():

// CompleteTransaction instructs backend to complete sending of a given transaction. // TODO(adam): investigate a possible bug that calling this method multiple times with the same Transaction ID // results in sending multiple transactions. func (m *Manager) CompleteTransaction(id common.QueuedTxID, password string) (hash gethcommon.Hash, err error) { log.Info("complete transaction", "id", id) tx, err := m.txQueue.LockInprogress(id) if err != nil { log.Warn("error getting a queued transaction", "err", err) return hash, err }

BTW, is it solving bug mentioned in TODO?

Oh yeah, this comment is not relevant anymore. The problem was that we were not tracking which transaction is in progress. The fix was introduced before this refactoring and we are still good so let's remove it.

I would just go with LockInProgress() error and use Get(). This code is not a bottleneck so readability vs preoptimization.

adambabik · 2018-01-22T17:22:59Z

geth/transactions/queue/queue.go

+	if err == nil {
+		q.remove(tx.ID)
+		tx.Hash = hash
+		tx.Done <- struct{}{}


What do you think about just closing the channel?

indeed looks like it is safe to close, i saw a reason for not closing it but i dont see it right now :)

oskarth · 2018-01-23T06:45:52Z

@rasom @oskarth, do you guys remember what was the purpose of eviction? Is it related to security, performance or something else? I don't like this idea because we discard transactions without informing the user about it.

@adambabik Sorry for the late reply. Nope, I have no idea.

adambabik

Overall looks great! Amazing job! I added just a few minor request changes, mostly in tests.

adambabik · 2018-01-23T14:42:49Z

geth/transactions/queue/queue.go

+}
+
+// LockInprogress returns transcation and locks it as inprogress
+func (q *TxQueue) LockInprogress(id common.QueuedTxID) (*common.QueuedTx, error) {


I can't find where it is used except for a test file.

If we have LockInProgress() and Get() and call it in this order everything should be ok without race conditions, right?

adambabik · 2018-01-23T14:46:33Z

geth/transactions/queue/queue.go

+		return
+	}
+	_, transient := transientErrs[err.Error()]
+	if !transient {


It can be written as if _, transient := transientErrs[err.Error()]; !transient {.

adambabik · 2018-01-23T14:48:50Z

geth/transactions/queue/queue_test.go

+	// enqueue will fail if transaction with hash will be enqueued
+	tx := common.CreateTransaction(context.Background(), common.SendTxArgs{})
+	tx.Hash = gethcommon.Hash{1}
+	s.Equal(ErrQueuedTxAlreadyProcessed, s.queue.Enqueue(tx))


Condition tx.Err != nil also should be checked.

adambabik · 2018-01-23T14:50:15Z

geth/transactions/queue/queue_test.go

+	err := keystore.ErrDecrypt
+	tx := s.testDone(hash, err)
+	s.Equal(keystore.ErrDecrypt, tx.Err)
+	s.NotEqual(hash, tx.Hash)


This looks like not needed as below we have s.Equal(gethcommon.Hash{}, tx.Hash).

adambabik · 2018-01-23T14:51:49Z

geth/transactions/queue/queue_test.go

+	// event is sent only if transaction was removed from a queue
+	select {
+	case <-tx.Done:
+	default:


Nice default usage 👍

divan · 2018-01-23T19:49:03Z

e2e/transactions/transactions_test.go


+	log.Info(fmt.Sprintf("Number of transactions sent: %d. Queue size (shouldn't be more than %d): %d",


I would remove all log lines from tests.
Perhaps it can be replaced with a check for the right txCount value here? (it's not used anywhere except this log line)

divan · 2018-01-23T19:51:51Z

geth/transactions/notifications.go

+	SendTransactionDiscardedErrorCode
+)
+
+var txReturnCodes = map[error]int{ // deliberately strings, in case more meaningful codes are to be returned


obsolete comment?

divan · 2018-01-23T19:54:04Z

geth/transactions/notifications.go

+
+// NotifyOnEnqueue returns handler that processes incoming tx queue requests
+func NotifyOnEnqueue(queuedTx *common.QueuedTx) {
+	log.Info("calling TransactionQueueHandler")


Looks like copypasted and forgotten log output. Remove it?

divan · 2018-01-23T20:04:54Z

geth/transactions/queue/queue.go

+	return nil, ErrQueuedTxIDNotFound
+}
+
+// LockInprogress returns transcation and locks it as inprogress


typo: transcation

divan

Minor remarks, LGTM 👍

Changes to TxQueue: - factored out into a separate module to ensure that only exported methods will be used - deleted EnqueuAsync as it is not used, it is always possible to restore it from git, if it will be ever needed - notifications moved to a manager, with the goal to simplify txqueue and separate concerns between manager and txqueue - simplified API for processing transactions After analyzing code I removed all redundant methods and currently all the processing can be done with Get and Done methods. - added inprogress map to store transactions that are taken for processing and removed Inprogress variable from transaction, the goal is to cleanup tx structure - removed Discard channel from transaction, transaction.Err can be used to understand if transaction was discarded or failed due to another error - transient errors stored as global variable of queue module for simplicity, it is unlikely that they will become dynamic Change to Manager: - simplified code that manages notifications, all handlers were removed and notifiers refactored to be simple functions that are called in two places, when transaction is queued and when manager finished waiting for transaction - notifications can be turned of by calling DisableNotifications on manager - made a function out of CreateTransaction method, as it can be used as a simple utility

adambabik

It's ready to be merged! 👏

adambabik · 2018-01-25T07:19:14Z

geth/transactions/txqueue_manager.go

 	log.Info("complete transaction", "id", queuedTx.ID)
-	var emptyHash gethcommon.Hash
+	log.Info("verifying account password for transaction", "id", queuedTx.ID)


This log is unnecessary as there is one above already.

adambabik · 2018-01-25T07:24:01Z

geth/transactions/txqueue_manager.go

 // QueueTransaction puts a transaction into the queue.
 func (m *Manager) QueueTransaction(tx *common.QueuedTx) error {
 	to := "<nil>"
 	if tx.Args.To != nil {
 		to = tx.Args.To.Hex()
 	}
 	log.Info("queue a new transaction", "id", tx.ID, "from", tx.Args.From.Hex(), "to", to)
+	err := m.txQueue.Enqueue(tx)
+	if m.notify {


It could be beneficial to create a method

func (m *Manager) notifyOnEnqueue(tx) { if m.notify { NotifyOnEnqueue(tx) } }

so that the whole logic when to send a notification is encapsulated in one place. Here, if there will be another condition, all NotifyOnEnqueue call conditions would need to be changed.

dshulyak · 2018-01-26T05:50:41Z

@adambabik @themue please, consider merging it

adambabik · 2018-01-26T06:00:03Z

@dshulyak done, sorry didn't notice it was a PR from a fork.

dshulyak force-pushed the queueref branch 2 times, most recently from 6e2128b to fe16b03 Compare January 4, 2018 12:17

dshulyak changed the title ~~[WIP] Refactoring of TxQueue and Manager~~ Refactoring of TxQueue and Manager Jan 4, 2018

dshulyak force-pushed the queueref branch 2 times, most recently from 4ada6ca to 775fb83 Compare January 4, 2018 13:19

dshulyak force-pushed the queueref branch from 775fb83 to a88f9eb Compare January 4, 2018 13:27

dshulyak commented Jan 5, 2018

View reviewed changes

adambabik suggested changes Jan 8, 2018

View reviewed changes

dshulyak force-pushed the queueref branch from 26e4c5c to f2848c7 Compare January 22, 2018 09:00

dshulyak mentioned this pull request Jan 22, 2018

Cover transaction manager with additional test cases #561

Closed

dshulyak force-pushed the queueref branch from f2848c7 to 820a67a Compare January 22, 2018 09:23

This was referenced Jan 22, 2018

Remove mocks for transaction manager and transaction queue #562

Merged

Result of tx processing returned as QueuedTxResult #537

Merged

adambabik suggested changes Jan 22, 2018

View reviewed changes

dshulyak force-pushed the queueref branch from cd24d17 to 4dd9212 Compare January 23, 2018 11:30

adambabik suggested changes Jan 23, 2018

View reviewed changes

divan reviewed Jan 23, 2018

View reviewed changes

divan approved these changes Jan 23, 2018

View reviewed changes

dshulyak force-pushed the queueref branch from 4dd9212 to 771bf2b Compare January 24, 2018 09:07

dshulyak added 2 commits January 24, 2018 12:30

Address review comments

9db329a

dshulyak force-pushed the queueref branch from 771bf2b to 9db329a Compare January 24, 2018 10:31

Lint warnings

e7171b5

dshulyak force-pushed the queueref branch from 9c6421d to 052999c Compare January 24, 2018 12:24

Rework lock inprogress method

052999c

adambabik approved these changes Jan 25, 2018

View reviewed changes

adambabik merged commit 680d051 into status-im:develop Jan 26, 2018


		func (m Manager) validateAccount(tx common.QueuedTx) (*common.SelectedExtKey, error) {


		log.Info(fmt.Sprintf("Number of transactions sent: %d. Queue size (shouldn't be more than %d): %d",

Refactoring of TxQueue and Manager #530

Refactoring of TxQueue and Manager #530

Conversation

dshulyak commented Jan 3, 2018 • edited Loading

dshulyak commented Jan 4, 2018 • edited Loading

divan commented Jan 4, 2018

dshulyak commented Jan 4, 2018 • edited Loading

dshulyak commented Jan 4, 2018

dshulyak commented Jan 4, 2018

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

dshulyak commented Jan 5, 2018 • edited Loading

adambabik commented Jan 7, 2018

dshulyak commented Jan 8, 2018

adambabik 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

dshulyak commented Jan 8, 2018 • edited Loading

dshulyak commented Jan 22, 2018

adambabik 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

dshulyak Jan 23, 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

divan Jan 23, 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

oskarth commented Jan 23, 2018

adambabik 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

divan left a comment

Choose a reason for hiding this comment

dshulyak commented Jan 3, 2018 •

edited

Loading

dshulyak commented Jan 4, 2018 •

edited

Loading

dshulyak commented Jan 4, 2018 •

edited

Loading

dshulyak commented Jan 5, 2018 •

edited

Loading

dshulyak commented Jan 8, 2018 •

edited

Loading

dshulyak Jan 23, 2018 •

edited

Loading

divan Jan 23, 2018 •

edited

Loading