server/{swap,market}: add funding checks for account-based assets #1293

buck54321 · 2021-11-15T00:31:45Z

Adds funding validation for account-based assets i.e. eth.

dex: Add RedeemSize field to Asset.

msgjson: Add RedeemSig field to Trade. RedeemSig should be
populated when the redemption is to an account-based asset.
This PR also indirectly proposes a protocol for specifying
the funding account. The account address should be utf-8 encoded
and passed as the coin ID of the only coin, along with the pubkey
and signature. The data signed will be the serialized msgjson.Limit or
msgjson.Market, with server stamp set to 0. Same signature input
for the RedeemSig.

*dex/order: The Trade type has methods for accessing the account address.

server/asset: AccountBalancer gets a ValidateSignature method.

general scheme: Orders and matches involving account-based assets are
indexed by Swapper and OrderPQ in the same way they are indexed for
users (account.AccountID). When an order comes into the order router,
stats for existing orders, including outstanding redemptions, is
fetched from all markets and the swapper. These stats are used with
the new order info to validate the users balance. The user must have
sufficient balance to cover all outstanding orders and redemptions.

server/book: All OrderPQ are given an AccountTracker set up for
any account-based assets, or none. The AccountTracker is essentially
the same as the userOrders index, but indexed by account address
instead. This is important because more than one user could be
using the same account.

AccountTracker has methods for iterating booked orders, exposed via
Book and Market methods used by Market as part of the MarketTunnel
interface method, PendingAccount, described below.

server/swap: Swapper gets an account index to mirror its user index.
The new AccountStats method returns information about in-process matches
that aren't yet swapped/redeemed for a particular asset.

server/market: OrderRouter is refactored for improved re-use between
handleLimit and handleMarket. For account based assets, OrderRouter
checks the MarketTunnels and MatchNegotiator for outstanding
order and match info, and then queries the backend (via DEXBalancer)
to see if the account has sufficient balance.

NewMarket handles balance checks on startup. This is accomplished
by additional tracking through the orders loop, and then a balance
check for account-based assets immediately before adding to the
order book. utxo-based asset handling is unchanged except that the
lot size compliance check it moved earlier in the loop.

chappjc · 2021-11-15T02:53:10Z

All looks sensible based on a quick scan through, although I didn't anticipate a diff this large for the task. :)

Not really complaints so much as a few things to note that jumped out at me:

Either the MatchNegotiator interface or its one AccountStats method could use some reconsideration of their name(s).
In general I am slightly off-put by the overt account awareness in all the non-asset server packages, esp. book, but I don't see a clearer solution or one that does not become very contrived to fit into the existing market machinery. I'd probably have gone the same way.
I thought we decided not to check the redeeming account balance and ownership server side, but I see the solution you proposed is not too complex. I was a bit taken aback by the specialized RedeemSig in the Trade message, but I think it's palatable - if we require proof of funding coins, it's not too wild to require proof that the receiving account is owned and funded as well. However, I do maintain that this is an unnecessary check. It's easy to pass the check temporarily to place an order, and causing a trade to fail after both swaps are broadcast means you've locked your funds too (already discourages this more on top of usual penalization).

Full review Mon/Tues. Let's try to get #1262 in asap.

buck54321 · 2021-11-15T09:42:07Z

I didn't anticipate a diff this large for the task. :)

More than 700 lines of the the net LOC gain is tests, if that makes it any better.

Either the MatchNegotiator interface or its one AccountStats method could use some reconsideration of their name(s).

I see that now. Will do.

In general I am slightly off-put by the overt account awareness in all the non-asset server packages, esp. book, but I don't see a clearer solution or one that does not become very contrived to fit into the existing market machinery. I'd probably have gone the same way.

I feel similarly. I got here because this is where the solution naturally evolved, and I think it's the right solution. It does touch some low-level, critical areas of code, but in fairly uncomplicated and auditable ways. For example, OrderPQ and Book now have methods for traversing the data, but all logic is kept in the new AccountPending method, and the accountTracker has no error return values or possible panics.

I thought we decided not to check the redeeming account balance and ownership server side

I didn't remember any specific conclusions on the matter, so sorry if this work somehow doesn't reflect a previous discussion. I do think it's in-line with the validation that the server already performs for utxo-based assets. We do our best to make sure that the user can complete their order when it's placed. An order redeeming to an account-based asset cannot be completed without funds to pay the redemption tx fees, and the taker would be left hanging if the maker can't redeem. IMO, the reason we check any of this is ensure a smooth experience for the other party, and that still applies here.

However, I do maintain that this is an unnecessary check.

The same could be said for funding coins for utxo-based assets. We could totally make it a free-for-all without any funding checks, and nobody's funds would be at risk.

martonp

This method looks good to me, but did you consider just keeping an integer record of how much funds are currently in use by each account, and then incrementing/decrementing as needed? The code would definitely be much simpler. I guess the downside is if the record somehow gets out of sync, it's not really possible to recover.

martonp · 2021-11-15T09:15:20Z

dex/calc/fees.go

@@ -14,12 +14,16 @@ import (
 // number of lots in the order. For the quote asset, maxSwaps is not swapVal /
 // lotSize, so it must be a separate parameter. The chained swap txns will be
 // the standard size as they will spend a previous swap's change output.
+// For account-based assets, inputsSize will be zero, and nfo.SwapSize =


I've created an issue regarding this: #1259
This is fine too, but I think it would be cleaner to have a new backend method.

As a side note, this difference isn't really due to ETH being account based, but because the fees are calculated based on gas instead of tx size.

I'm fine with having custom functions for dealing with account-based asset required funds. Outside of the asset backends, OrderRouter is the only user of RequiredOrderFunds and RequiredOrderFundsAlt, and OrderRouter is already account-aware (in this work) so the functions are only used in a context where we know already know what kind of asset we're dealing with. So we should be free to diverge these calc functions with little ripple-effect. I'm not 100% certain what you mean by "new backend method". Do you mean to calculate the input size? If so, there's really nothing to do. Since OrderRouter already knows it's account-based, it never collects the asset.FundingCoin (only returned from asset.OutputTracker anyway), which is where the input size comes from.

martonp · 2021-11-15T11:19:33Z

server/market/market.go

+					log.Errorf("rejecting account-based-base-asset order %s that has no coins ¯\\_(ツ)_/¯", lo.ID())
+					continue ordersLoop
+				}
+				addr = string(lo.Coins[0])


This would be an encoded AmountCoinID, not just an address.

As we wiggle everything into place, one of the things we need to do is to "minimize" the API by eliminating any extraneous information. The server doesn't need the value encoded in the coin ID. It's knows how much to lock for the order.

Just looking forward, I would expect a number of changes to the existing server-side coin ID system. I'll comment in more detail on #1154 today so we can plan it out together. I sort of feel bad for not being more involved with the details of the coin IDs earlier, but it was so early in the process and I hadn't quite wrapped my head around everything. I'm getting there now.

martonp · 2021-11-15T12:02:22Z

What we discussed was that the server just needs to check things that if incorrectly done by one user would negatively affect another user. In this case, not having enough funds to redeem a swap would only negatively affect the user making the mistake, so this check could just be handled by the client code.

buck54321 · 2021-11-15T12:35:09Z

In this case, not having enough funds to redeem a swap would only negatively affect the user making the mistake, so this check could just be handled by the client code.

If the maker fails to redeem, the taker has stuck funds.

buck54321 · 2021-11-15T12:56:40Z

In this case, not having enough funds to redeem a swap would only negatively affect the user making the mistake, so this check could just be handled by the client code.

If the maker fails to redeem, the taker has stuck funds.

Just some additional shower thoughts on this...

So technically, we don't need to check it if it's a market order or limit order with time-in-force = immediate, since the taker cannot inconvenience the maker and can only shoot themselves in the foot. This means that nobody can really point at DEX when things go wrong,

BUT

they will anyway. And while I agree it's not the server's job to write the client software, that won't stop some noob using a crappy or misconfigured client from running around screaming "DEX locked my money in a contract and wants me to pay to get it out". Screw that guy and his crappy client, obviously, but my point is that we maintain an interest in providing a smooth user experience and loss of funds is not the only bad thing that can happen to slow DEX adoption.

server/asset/eth/rpcclient.go

JoeGruffins

Tests fine for me for UTXO coins. No real complaints.

JoeGruffins · 2021-11-19T00:30:07Z

server/asset/eth/eth.go

 // need to be per transaction. Setting this to zero produces the expected
 // result in fee calculations.
 func (eth *Backend) InitTxSizeBase() uint32 {
-	return 0
+	return InitGas
 }


Doc still says set to zero.

JoeGruffins · 2021-11-19T00:50:01Z

server/market/balancer.go

+	matchNegotiator MatchNegotiator
+}
+
+// NewDEXBalancer is a constructor for a BackedBalancer. Provided assets will


constructor for a DEXBalancer?

JoeGruffins · 2021-11-19T00:56:09Z

server/market/balancer.go

+	assetInfo := backedAsset.assetInfo
+	redeemCosts := uint64(redeems) * assetInfo.RedeemSize * assetInfo.MaxFeeRate
+	reqFunds := calc.RequiredOrderFunds(qty, 0, lots, assetInfo) + redeemCosts
+


Maybe add a trace log with some info about the account and balances for now?

JoeGruffins · 2021-11-19T01:09:42Z

server/market/market.go

+		log.Debugf("Checking %d base asset (%d) balances.", len(baseAcctStats), base)
+		for acctAddr, stats := range baseAcctStats {
+			if !cfg.Balancer.CheckBalance(acctAddr, mktInfo.Base, stats.qty, stats.lots, stats.redeems) {
+				log.Errorf("%s base asset account failed the startup balance check on the %s market", acctAddr, mktInfo.Name)


Is this error level? The account's balance could be too low through no fault of the server. I might even say info.

JoeGruffins · 2021-11-19T02:02:14Z

server/market/orderrouter.go

@@ -474,21 +366,10 @@ func (r *OrderRouter) handleMarket(user account.AccountID, msg *msgjson.Message)
 	var commit order.Commitment
 	copy(commit[:], market.Commit)

-	fundingAsset := assets.funding
+	// fundingAsset := assets.funding


JoeGruffins · 2021-11-19T02:09:13Z

server/market/orderrouter.go

+	receivingBalancer, isToAccount := assets.receiving.Backend.(asset.AccountBalancer)
+	if isToAccount {
+		if redeemSig == nil {
+			log.Errorf("user %s did not include a RedeemSig for received asset %s", user, assets.receiving.Symbol)


Are all of the new logs here really error level? I suggest warn, or info, as they are not really a server problem.

I'm even thinking DBG for these, but we can keep INF for now I guess. Anything that a user can do at will with little cost shouldn't be too chatty in the logs.

server/asset/eth/rpcclient.go

server/market/market.go

server/market/balancer.go

chappjc · 2021-12-01T17:17:29Z

server/market/balancer.go

+}
+
+// NewDEXBalancer is a constructor for a DEXBalancer. Provided assets will
+// be filtered for those that are account-based. The matchNegotitator is


matchNegotiator

server/market/orderrouter.go

chappjc · 2021-12-01T17:19:20Z

server/market/market.go

+// AccountPending sums the orders quantities that pay to or from the specified
+// account address.


Could stand to describe redeems int as well.

server/dex/dex.go

server/market/orderrouter.go

chappjc · 2021-12-01T20:43:50Z

server/market/orderrouter.go

+			return msgjson.NewError(msgjson.OrderParameterError, "account-type asset funding requires exactly one coin ID")
+		}
+		acctProof := coins[0]
+		if len(acctProof.PubKeys) != 1 || len(acctProof.Sigs) != 1 || len(acctProof.Redeem) > 0 {


Working through things here:

The "coin ID" in both acctProof.ID and trade.Coins[0] are equal, as evident in the callers, handleMarket and handleLimit who populate the order.Trade.Coins from each msgjson.{Market,Limit}Order.Trade.Coins[i].ID

These coin ID encodings deviate from the new transaction id/hash convention for server-side ETH coin IDs being established in server/asset/eth: update server coin ID parsing #1307. Hence ValidateCoinID cannot be used on them.

They are also the same as order.(*Trade).FromAccount before hex string encoding.

order.(*Trade).ToAccount is the same as order.Trade.Address and thus order.(*Trade).SwapAddress

Mostly I want to be sure this is all the intended design and I'm not missing anything.

That mostly seems right. The term "coin ID" is so general now as to be confusing. The definition of coin ID need to be evaluated separately for each message and asset.

ValidateCoinID is in a weird place right now, I guess. It's used for funding/swap/redeem coins for utxo-based assets, but only for swap/redeem coins for acct-based. I wouldn't mind breaking that method into as many as 3 separate methods, ValidateSwapCoin, ValidateRedeemCoin, and ValidateFundingCoin (OutputTracker-only). We could also combine the swap and redeem methods into something like ValidateTradeCoin for now. Or we could just leave it. It's all terrible.

Let's solve with docs for now rather than end up with a ton of new methods and wrappers and stuff

dex/order/order.go

buck54321 · 2021-12-01T22:41:20Z

server/asset/common.go

+	// ValidateSignature checks that the pubkey is correct for the address and
+	// that the signature shows ownership of the associated private key.
+	ValidateSignature(addr string, pubkey, msg, sig []byte) error


Thinking of updating the docs here to direct enforcement of an address standard.

// ValidateSignature checks that the pubkey is correct for the address and // that the signature shows ownership of the associated private key. The // address string will be validated against a strict standard format. For // example, ethereum address should have the "0x" prefix and be lower-case. // Any other format should return an error.

ValidateSignature, being an ETH backend method, shouldn't have trouble with any of the normal string encodings.

common.HexToAddress handles all valid variants regardless of case or 0x prefix.
https://pkg.go.dev/github.com/ethereum/go-ethereum/common#HexToAddress
In the recent update to OwnsAddress I followed the example from https://github.com/ethereum/go-ethereum/blob/7a0c19f813e285516f4b525305fd73b625d2dec8/common/types.go#L383 where it first validates with IsHexAddress, then essentially does HexToAddress.

But this string encoding is used as a unique ID throughout dcrdex.

Oh, it certainly needs to be standardized for all the other uses, yes, I just don't think ValidateSignature is the place where it matters and hence not the best place to doc the standard we pick.

ValidateSignature is run on all incoming orders. It seems like a great place to validate the address, no?

I'm not sure we're on the same page. I'm trying to instruct asset backend developers to validate the address string against a strict standard. I'm just using ethereum as an example.

This is under the assumption that we use the scheme defined in this PR, which is that the address is either the msgjson.Trade.Address or string(msgjson.Trade.Coins[0]).

Alright, well since order router and ValidateSignature are the initial validation points for the address string before it ends up playing a role as a map key in both book and swap, I suppose have it be strict about the encoding, more strict than it itself would require?

That's what I'm trying to get at. Can improve the language, it seems.

Another approach would be for ValidateSignature to return a canonicalized address string after processing whatever string came in via the msgjson.Trade fields. Would likely be unconverted for dcr or btc, but eth could accept any of the various encodings and spit out maybe just 0x{lowercasehex}?

The fact that msgjson.Trade.Address has always been a string and that's hard to change now kind means we have to have stick with a string for the other address, but @davecgh did have a nice suggestion if we had bytes that could be put in a type like type Address interface { fmt.Stringer; Bytes() []byte }, but alas the issue is upstream of that with the communications type committing us to a string. So I think we have to work with it as-is.

server/swap/swap.go

server/market/orderrouter.go

chappjc · 2021-12-02T21:17:38Z

server/asset/common.go

+	// ValidateSignature checks that the pubkey is correct for the address and
+	// that the signature shows ownership of the associated private key.
+	ValidateSignature(addr string, pubkey, msg, sig []byte) error


Another approach would be for ValidateSignature to return a canonicalized address string after processing whatever string came in via the msgjson.Trade fields. Would likely be unconverted for dcr or btc, but eth could accept any of the various encodings and spit out maybe just 0x{lowercasehex}?

The fact that msgjson.Trade.Address has always been a string and that's hard to change now kind means we have to have stick with a string for the other address, but @davecgh did have a nice suggestion if we had bytes that could be put in a type like type Address interface { fmt.Stringer; Bytes() []byte }, but alas the issue is upstream of that with the communications type committing us to a string. So I think we have to work with it as-is.

Adds funding validation for account-based assets i.e. eth. dex: Add RedeemSize field to Asset. msgjson: Add RedeemSig field to Trade. RedeemSig should be populated when the redemption is to an account-based asset. This PR also indirectly proposes a protocol for specifying the funding account. The account address should be utf-8 encoded and passed as the coin ID of the only coin, along with the pubkey and signature. The data signed will be the serialized msgjson.Limit or msgjson.Market, with server stamp set to 0. Same signature input for the RedeemSig. dex/order: The Trade type has methods for accessing the account address. server/asset: AccountBalancer gets a ValidateSignature method. general scheme: Orders and matches involving account-based assets are indexed by Swapper and OrderPQ in the same way they are indexed for users (account.AccountID). When an order comes into the order router, stats for existing orders, including outstanding redemptions, is fetched from all markets and the swapper. These stats are used with the new order info to validate the users balance. The user must have sufficient balance to cover all outstanding orders and redemptions. server/book: All OrderPQ are given an AccountTracker set up for any account-based assets, or none. The AccountTracker is essentially the same as the userOrders index, but indexed by account address instead. This is important because more than one user could be using the same account. AccountTracker has methods for iterating booked orders, exposed via Book and Market methods used by Market as part of the MarketTunnel interface method, PendingAccount, described below. server/swap: Swapper gets an account index to mirror its account index. The new AccountStats method returns information about in-process matches that aren't yet swapped/redeemed for a particular asset. server/market: OrderRouter is refactored for improved re-use between handleLimit and handleMarket. For account based assets, OrderRouter checks the MarketTunnels and MatchNegotiator for outstanding order and match info, and then queries the backend (via DEXBalancer) to see if the account has sufficient balance. NewMarket handles balance checks on startup. This is accomplished by additional tracking through the orders loop, and then a balance check for account-based assets immediately before adding to the order book. utxo-based asset handling is unchanged except that the lot size compliance check it moved earlier in the loop.

chappjc

Almost certainly my last review. Mainly nits at this point.

server/swap/swap.go

server/market/balancer.go

server/market/market.go

buck54321 · 2021-12-04T15:09:01Z

@chappjc I started moving accountTracker to Book, but it fell apart because the Remove method only takes and order ID and the RemoveUserOrders method only takes a DEX AccountID, so accountTracker would need to add removal methods and possibly indices for both, which would largely mimic the logic already present in OrderPQ's methods. Leaving it seems like a better option to me.

chappjc · 2021-12-04T15:25:20Z

@chappjc I started moving accountTracker to Book, but it fell apart because the Remove method only takes and order ID and the RemoveUserOrders method only takes a DEX AccountID, so accountTracker would need to add removal methods and possibly indices for both, which would largely mimic the logic already present in OrderPQ's methods. Leaving it seems like a better option to me.

Thanks for looking into it. What led me to suggest the change was because it looks like the returns from the OrderPQ methods Remove and RemoveUserOrders both return any *order.LimitOrders that were removed, while Insert says if it was added.

Sorry if I'm still missing a detail, but roughly here's what I figure would work to keep the OrderPQ out of it:

func (b *Book) Insert(o *order.LimitOrder) bool {
	if o.Quantity%b.lotSize != 0 {
		log.Warnf("(*Book).Insert: Refusing to insert an order with a quantity that is not a multiple of lot size.")
		return false
	}
	b.mtx.Lock()
	defer b.mtx.Unlock()
	if o.Sell {
		if b.sells.Insert(o) {
			b.acctTracker.add(o)
			return true
		}
		return false
	}
	if b.buys.Insert(o) {
		b.acctTracker.add(o)
		return true
	}
	return false
}

func (b *Book) Remove(oid order.OrderID) (*order.LimitOrder, bool) {
	b.mtx.Lock()
	defer b.mtx.Unlock()
	if removed, ok := b.sells.RemoveOrderID(oid); ok {
		b.acctTracker.remove(removed)
		return removed, true
	}
	if removed, ok := b.buys.RemoveOrderID(oid); ok {
		b.acctTracker.remove(removed)
		return removed, true
	}
	return nil, false
}

func (b *Book) RemoveUserOrders(user account.AccountID) (removedBuys, removedSells []*order.LimitOrder) {
	b.mtx.Lock()
	defer b.mtx.Unlock()
	removedBuys = b.buys.RemoveUserOrders(user)
	for _, lo := range removedBuys {
		b.acctTracker.remove(lo)
	}
	removedSells = b.sells.RemoveUserOrders(user)
	for _, lo := range removedSells {
		b.acctTracker.remove(lo)
	}
	return
}

func (b *Book) IterateBaseAccount(acctAddr string, f func(lo *order.LimitOrder)) {
	b.mtx.RLock()
	defer b.mtx.RUnlock()
	b.acctTracker.iterateBaseAccount(acctAddr, f)
}

func (b *Book) IterateQuoteAccount(acctAddr string, f func(lo *order.LimitOrder)) {
	b.mtx.RLock()
	defer b.mtx.RUnlock()
	b.acctTracker.iterateQuoteAccount(acctAddr, f)
}

Perhaps I'm incorrectly lumping the two accountTracker's together?

chappjc · 2021-12-04T16:18:03Z

I made a mess of the tests, but it seems to work: 4264adf
Is it wrong to have a single accountTracker?

chappjc

Looks good. Will run some interactive tests and wait for any possible other review follow-ups

chappjc · 2021-12-05T16:37:19Z

I said I was gonna let that sit a while longer, but we've been over it plenty and we have a backlog of ETH PRs stacking up.

martonp reviewed Nov 15, 2021

View reviewed changes

chappjc mentioned this pull request Nov 15, 2021

[for discussion] eth: locked amount accounting updates #1289

Closed

martonp reviewed Nov 17, 2021

View reviewed changes

server/asset/eth/rpcclient.go Outdated Show resolved Hide resolved

JoeGruffins approved these changes Nov 19, 2021

View reviewed changes

chappjc modified the milestone: 0.4 Nov 22, 2021

chappjc added the ETH label Nov 22, 2021

buck54321 force-pushed the acct-based-funding branch from fb2609e to eae5215 Compare November 23, 2021 10:06

martonp mentioned this pull request Nov 24, 2021

eth/client: Don't allow submitting a trade if not enough balance to redeem #1275

Closed

chappjc added this to the 0.5 milestone Nov 28, 2021

buck54321 force-pushed the acct-based-funding branch from eae5215 to bc8d9f1 Compare November 30, 2021 00:04

chappjc reviewed Dec 1, 2021

View reviewed changes

server/asset/eth/rpcclient.go Show resolved Hide resolved

buck54321 force-pushed the acct-based-funding branch from 3981af9 to 5cf469a Compare December 1, 2021 12:59

chappjc reviewed Dec 1, 2021

View reviewed changes

server/market/market.go Show resolved Hide resolved

chappjc requested changes Dec 1, 2021

View reviewed changes

chappjc mentioned this pull request Dec 1, 2021

server/asset/eth: update server coin ID parsing #1307

Merged

chappjc reviewed Dec 1, 2021

View reviewed changes

dex/order/order.go Show resolved Hide resolved

buck54321 force-pushed the acct-based-funding branch from 5cf469a to adc6809 Compare December 1, 2021 21:53

buck54321 commented Dec 1, 2021

View reviewed changes

chappjc requested changes Dec 2, 2021

View reviewed changes

buck54321 added 4 commits December 3, 2021 04:32

smarter balance

90edbd5

more router tests. market buy bug fix

fe3823f

separate PendingAccounter interface

55bd7f0

buck54321 added 3 commits December 3, 2021 04:32

doc updates

1bd0753

doc address format enforcement requirements

f53f133

review followup

ae1af1a

buck54321 force-pushed the acct-based-funding branch from 10a9466 to ae1af1a Compare December 3, 2021 10:48

chappjc requested changes Dec 3, 2021

View reviewed changes

server/swap/swap.go Outdated Show resolved Hide resolved

server/swap/swap.go Show resolved Hide resolved

server/market/balancer.go Show resolved Hide resolved

server/market/market.go Show resolved Hide resolved

review followup

80ac931

leave OrderPQ alone

db76046

buck54321 force-pushed the acct-based-funding branch from a367380 to db76046 Compare December 4, 2021 18:06

chappjc approved these changes Dec 5, 2021

View reviewed changes

chappjc merged commit 516d114 into decred:master Dec 5, 2021

chappjc mentioned this pull request Dec 10, 2021

eth/client/simnet_tests: testSendSignedTransaction failing. #1342

Closed

		// AccountPending sums the orders quantities that pay to or from the specified
		// account address.

server/{swap,market}: add funding checks for account-based assets #1293

server/{swap,market}: add funding checks for account-based assets #1293

Conversation

buck54321 commented Nov 15, 2021 • edited

chappjc commented Nov 15, 2021 • edited

buck54321 commented Nov 15, 2021 • edited

martonp 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

martonp commented Nov 15, 2021

buck54321 commented Nov 15, 2021

buck54321 commented Nov 15, 2021

JoeGruffins 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

chappjc Dec 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 Dec 1, 2021 • edited

Choose a reason for hiding this comment

chappjc Dec 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 Dec 1, 2021 • edited

Choose a reason for hiding this comment

buck54321 Dec 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Dec 2, 2021 • edited

Choose a reason for hiding this comment

chappjc Dec 2, 2021 • edited

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

buck54321 commented Dec 4, 2021

chappjc commented Dec 4, 2021

chappjc commented Dec 4, 2021

chappjc left a comment

Choose a reason for hiding this comment

chappjc commented Dec 5, 2021

buck54321 commented Nov 15, 2021 •

edited

chappjc commented Nov 15, 2021 •

edited

buck54321 commented Nov 15, 2021 •

edited

chappjc Dec 1, 2021 •

edited

buck54321 Dec 1, 2021 •

edited

chappjc Dec 1, 2021 •

edited

buck54321 Dec 1, 2021 •

edited

buck54321 Dec 1, 2021 •

edited

chappjc Dec 1, 2021 •

edited

chappjc Dec 2, 2021 •

edited

chappjc Dec 2, 2021 •

edited