rpc: event subscription management for RPC client #516

thanethomson · 2020-08-12T01:31:32Z

Closes #313

In the spirit of our session today, I'm building and testing this alongside developing ADR #476 at the moment (it diverges slightly from the ADR right now, but I will update the ADR tomorrow).

After several iterations, I think I've come up with a subscription management interface and a transport abstraction layer that could mostly work for our purposes.

Since this is a pretty large PR, the quickest way to get into it is to first read the updated crate docs for the tendermint-rpc package:

cd ./rpc/
cargo doc --all-features --open

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md

Signed-off-by: Thane Thomson <connect@thanethomson.com>

This version of the transport provides a request matching interface, along with an implementation of a request method-based matcher (produces specific fixed responses according to the request's method). Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

This commit adds the bulk of the subscription management machinery as well as a rudimentary integration test (which should be marked as `#[ignore]` ASAP) just to demonstrate obtaining new block events from a Tendermint instance running locally with default settings. Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

xla

Had a first pass, left a bunch of comments regarding structure and style inline.

Generally this shapes up to be quite nice. The main concern I'd like to raise is that it seems a rather big amount of abstractions. There are a wide array of indirections and things that have to be plugged together to make the machinery work, i.e. Manager, Router, Subscription, SubscriptionTransport, EventConnection, EventProducer and so on. Another indicator that it's not yet the entirely correct set of abstractions is if you follow the imports it looks like deeply nested modules import entities from their top-level module. It would be nice to come to a more concise and clear hierarchy. Happy to discuss in more detail what I think could be reduced. Generally it would help to think along the lines of what needs to be injected/configurable for different environments.

rpc/Cargo.toml

rpc/src/client.rs

xla · 2020-08-12T15:30:29Z

rpc/src/client/subscription.rs

+#[derive(Debug)]
+pub struct Subscription {
+    id: SubscriptionId,
+    query: String,


Should this not be a native Query type?

I actually had implemented a reasonable-looking query implementation in one of my previous iterations, so I'll bring it back in here. I just wanted some feedback on the overarching architecture first prior to adding in more details.

rpc/src/client/subscription.rs

rpc/src/client/transport.rs

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

thanethomson · 2020-08-12T16:00:57Z

Had a first pass, left a bunch of comments regarding structure and style inline.

Generally this shapes up to be quite nice. The main concern I'd like to raise is that it seems a rather big amount of abstractions. There are a wide array of indirections and things that have to be plugged together to make the machinery work, i.e. Manager, Router, Subscription, SubscriptionTransport, EventConnection, EventProducer and so on. Another indicator that it's not yet the entirely correct set of abstractions is if you follow the imports it looks like deeply nested modules import entities from their top-level module. It would be nice to come to a more concise and clear hierarchy. Happy to discuss in more detail what I think could be reduced. Generally it would help to think along the lines of what needs to be injected/configurable for different environments.

Thanks @xla! I actually had an idea this morning during our onboarding as to how to refactor this a bit to reduce the number of abstractions. For one thing, right now there are effectively two asynchronous processes (outside of the originating task) facilitating the subscription mechanism:

A SubscriptionRouter, whose only job is to listen for incoming Events and route them to Subscriptions.
A transport driver (in the standard case, a WsDriver for the WebSocket connection).

I've realized I can probably reduce this logic to just one async task (the WsDriver) by embedding the routing logic, and I've got some ideas as to how to do this elegantly.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

The previous iteration of the subscription mechanism relied on two asynchronous tasks to manage subscriptions and route events from the WebSockets connection to their relevant subscribers. This version only uses one async task (the WebSocket driver, which must interact asynchronously with the connection). In addition to this, I tried experimenting with using generics instead of trait objects for the transport layer for the `Client`. This works, but the downside of using generics is that, if you want generalizability at layers higher up, it comes at the cost of decreased readability of the code. Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

After much consideration, I decided to do away with the `Transport` and related traits entirely. It makes more sense, and results in better ability to test, if we abstract the `Client` interface instead. This also allows for greater flexibility when implementing different kinds of transports, while still allowing for lazy/optional instantiation of the subscription mechanism. Additional integration tests are included here, as well as additional documentation. Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson · 2020-08-14T17:16:24Z

While I work on updating #476, the quickest way to get a feel for the new architecture here is to clone this branch locally and take a look at the docs:

cd ./rpc/
cargo doc --all-features --open

Signed-off-by: Thane Thomson <connect@thanethomson.com>

* Add first draft of ADR-008 Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add comment about WebSockets connection Signed-off-by: Thane Thomson <connect@thanethomson.com> * Rename to "RPC Client" for greater clarity Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update docs/architecture/adr-008-event-subscription.md Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com> * Update to reflect work on #516 Signed-off-by: Thane Thomson <connect@thanethomson.com> * Leave out implementation details (ADR) Signed-off-by: Thane Thomson <connect@thanethomson.com> * Clarify handle/driver concurrency model Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update with latest architecture inspired by #516 Signed-off-by: Thane Thomson <connect@thanethomson.com> * Condense Client trait example for readability Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reduce complexity of subscription interface Signed-off-by: Thane Thomson <connect@thanethomson.com> * Introduce ClosableClient trait Signed-off-by: Thane Thomson <connect@thanethomson.com> * Reduce number of client implementations Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove SubscriptionRouter from ADR discussion (it is an implementation detail) Signed-off-by: Thane Thomson <connect@thanethomson.com> * Update ERD to reflect current architecture Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add note on bounded vs unbounded channels in subscriptions Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix link for async drop discussion Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add more structure and explanation to the Subscription section Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add Subscription section on leveraging futures::stream utils Signed-off-by: Thane Thomson <connect@thanethomson.com> * Clarify ERD by using colours instead of line thickness Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

Merges the latest changes from `master` while adjusting for Tendermint v0.34 compatibility. Signed-off-by: Thane Thomson <connect@thanethomson.com>

rpc/src/client/subscription.rs

Signed-off-by: Thane Thomson <connect@thanethomson.com>

romac · 2020-09-15T13:34:16Z

@thanethomson Just pushed a fix for the last remaining WASM build error.

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson · 2020-09-15T13:53:04Z

@thanethomson Just pushed a fix for the last remaining WASM build error.

Awesome, thanks @romac! 👍

ebuchman

Looks like great work. Finally got a chance to at least read through it all but still some pieces I want to look more closely at.

While the ADR diagram is helpful for outlining the components, I think an even more helpful diagram might be one of the concurrency, showing what the different threads are and how the channel connectivity works and who's sending msgs to whom

ebuchman · 2020-09-15T01:11:04Z

light-client/Cargo.toml

@@ -28,7 +28,7 @@ secp256k1 = ["tendermint/secp256k1", "tendermint-rpc/secp256k1"]

 [dependencies]
 tendermint = { version = "0.16.0", path = "../tendermint" }
-tendermint-rpc = { version = "0.16.0", path = "../rpc", default-features = false }
+tendermint-rpc = { version = "0.16.0", path = "../rpc", features = ["client", "transport_http"] }


I think we might want to preserve this default-features = false and list the features above (line 26)?

Also what's the significant of the transport_http feature? When would someone not want it?

Fixed this 👍

As for the transport_http feature, I'm not sure. Are there instances where someone may want to exclusively use WebSocket-based subscription, but not perform any other requests via the HTTP interface?

Also, when we implement a WebSocket-based RPC client for the rest of the RPC methods (i.e. those currently only supported via HTTP), would we want to include the HTTP client by default?

I think it's a question about dependencies. I would expect websockets to include a superset of the http dependencies, or at least the majority ++. If there's any extra deps in the http client that are not in the websocket client I'd expect them to be minimal and so the cost of just bundling them would be small.

So,

Are there instances where someone may want to exclusively use WebSocket-based subscription, but not perform any other requests via the HTTP interface?

Probably, but not sure it's worth the feature distinction to have websockets without the http.

That said we may want to preserve http without websockets.

So I would imagine three layers of use cases:

default (no http or websockets, just rpc types)

http (no websockets)

full (websockets, http, everything)

Hopefully that would simplify things. In general for now I think we should air on the side of simplicity and consolidation rather than flexibility and choice, unless we have strong existing demand/reason for flexibility in certain places.

So the HttpClient requires the use of the hyper and http crates, whereas the WebSocket-based subscription client doesn't.

If you'd like to simplify, can I suggest doing away with the subscription feature and shortening the feature names:

Flag Description Additional Dependencies

default Just the RPC types None

client All the client-related traits, but no implementations async-trait, futures, tokio

http HttpClient (implements Client) http, hyper, tokio

websocket WebSocketSubscriptionClient, and in future a WebSocketClient, which will implement the Client and SubscriptionClient traits and will supersede the WebSocketSubscriptionClient async-tungstenite, tokio

mock MockClient and MockSubscriptionClient tokio

Perhaps it's worth it here to rename the existing WebSocketSubscriptionClient to WebSocketClient, so its name doesn't change in future?

Also, we could do away with the mock clients if you want. They were useful in my testing, but it's up to downstream users (e.g. IBC) to say if they'd see value in such a mock client.

For posterity, we've simplified this even further now, as per #569

ebuchman · 2020-09-15T01:14:39Z

light-client/src/components/io.rs

            let peer_addr = self.peer_map.get(&peer).unwrap().to_owned();
-            rpc::Client::new(peer_addr)
+            Ok(rpc::HttpClient::new(peer_addr).map_err(IoError::from)?)


Maybe pedantic but shouldn't the PeerId type already hold a valid address and hence this shouldn't need to return an error?

Technically a Tendermint Address could contain either a TCP or a Unix socket address, and we need the host/port details in order to connect to the RPC endpoint.

We could do away with the need for an error here by requiring host/port details instead of a peer_addr, or we could just panic if a user supplies a Unix socket address?

light-client/src/components/io.rs

ebuchman · 2020-09-15T01:18:21Z

rpc/Cargo.toml

+client = [ "async-trait", "futures" ]
+secp256k1 = [ "tendermint/secp256k1" ]
+subscription = [ "tokio" ]
+transport_http = [ "http", "hyper", "tokio" ]


Maybe this should just be consolidated with client? How much benefit do we really get by opting for the transport_mock over the transport_http ?

ebuchman · 2020-09-15T01:22:30Z

rpc/src/client.rs

-    ) -> Result<abci_query::AbciQuery, Error> {
+    ) -> Result<abci_query::AbciQuery>
+    where
+        V: Into<Vec<u8>> + Send,


Not sure why we needed this to be impl Trait in the first place but the added type bound suggests maybe we should reconsider? Could it not just be Vec from the get go?

Not necessarily to address in this PR but follow up along with the Height comment

ebuchman · 2020-09-15T02:01:56Z

rpc/src/client/transport/websocket.rs

+/// ## Examples
+///
+/// ```rust,ignore
+/// use tendermint_rpc::{WebSocketSubscriptionClient, SubscriptionClient, ClosableClient};


Would be great if we only needed one import here but I guess there's no way to avoid needing to explicitly import the traits?

I'd love to have a way to do away with these extra imports, just not sure how. @romac do you perhaps know of some kind of trick to consolidate these imports? (I can only think of hairy macro-based ones)

ebuchman · 2020-09-15T02:13:09Z

rpc/src/client/transport/websocket.rs

+            return Ok(());
+        }
+        self.router
+            .pending_add(id.as_ref(), &id, query, event_tx, result_tx);


What is the diff between id_as_ref() and &id and should these two things really be coming from the same id ?

This was technically an implementation of the AsRef trait to provide a &str reference, but looking at it now it's probably somewhat ambiguous. I've now implemented an as_str() method to return a &str instead, which I think should be clearer.

But it's also a bit jarring that these two distinct Ids are actually the same. Is that intended? Maybe could be documented/clarified somewhere the subscription id will be drawn from the json-rpc request id ?

Originally I had them both be exactly the same, but then I ran into an issue where I needed to send unsubscribe requests with their own request ID (distinct from the subscription ID) to ensure that the response from the remote endpoint is, in fact, exclusively relevant to the unsubscribe request. Otherwise we may confuse an error meant for the subscription with one meant for the unsubscribe request.

So subscribe requests' IDs are the same as the subscription ID (to correlate incoming events, and possibly errors, relating to the subscription), but unsubscribe requests' IDs need to be distinct.

rpc/src/event.rs

tendermint/src/abci/responses.rs

ebuchman · 2020-09-15T02:25:52Z

tendermint/tests/integration.rs

-    pub fn localhost_rpc_client() -> Client {
-        Client::new("tcp://127.0.0.1:26657".parse().unwrap())
+    pub fn localhost_rpc_client() -> HttpClient {
+        HttpClient::new("tcp://127.0.0.1:26657".parse().unwrap()).unwrap()


Seems like there's an unnecessary extra .unwrap() here from an API perspective, no? Related to my other PeerId comment

romac · 2020-09-15T14:28:02Z

For some reason, I cannot reply inline in the PR, so here it is:

@ebuchman Do we have any use cases for non http transport? I'm not sure anything else is even supported by Tendermint. I suppose it could be https but not sure if that would need entirely new transport...

Yes, we might also want to have a Fetch-based transport for running the light client against a full node directly from the browser via WebAssembly.

…clarity Signed-off-by: Thane Thomson <connect@thanethomson.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

ebuchman

A few more thoughts while I continue to dive into this.

If we bring back the old event listener, then I think it's fine to merge this (ie. it should be mostly non-breaking) and then we can continue to review and improve it from master

ebuchman · 2020-09-15T18:14:37Z

rpc/src/client/event_listener.rs

@@ -1,216 +0,0 @@
-//! Tendermint Websocket event listener client


Maybe we should bring this back for now so folks can upgrade their websocket code at their own pace?

The trouble with bringing this back is that the types exposed by the event listener conflict with the types I've added in rpc/src/event.rs. As far as I can see in IBC it's just this code that needs to be updated: https://github.com/informalsystems/ibc-rs/blob/master/relayer/src/event_monitor.rs

Ok fair enough. Let's merge this as is and once ibc is ready to upgrade to our master we can work on integration there.

ebuchman · 2020-09-15T18:41:59Z

rpc/src/client/transport/websocket.rs

+                        ),
+                    ),
+                },
+                Some(cmd) = self.cmd_rx.recv() => match cmd {


I find it a bit confusing that subscriptions come over the cmd_rx channel but the unsubscriptions come over a separate terminate channel. Why not give the Susbcription objects the cmd_tx and let them send Unsubscribe commands over that?

ebuchman · 2020-09-15T18:48:33Z

rpc/src/client/transport/websocket.rs

+    }
+
+    async fn run(mut self) -> Result<()> {
+        // TODO(thane): Should this loop initiate a keepalive (ping) to the


I believe so. At least the Go client does this and I think it's proper behaviour for a websocket client

ebuchman · 2020-09-15T18:53:59Z

rpc/src/client/transport/websocket.rs

+        loop {
+            tokio::select! {
+                Some(res) = self.stream.next() => match res {
+                    Ok(msg) => self.handle_incoming_msg(msg).await?,


So I'm trying to understand what benefit we're get from using async/await, if any. It seems that pretty much every possibly async function call has an await immediately after, so everything in this run method is effectively happening synchronously. Is that true? It seems it may apply more broadly than just this run method, but since this runs in it's own thread and is relatively contained, it's easier to analyze.

I was also somewhat surprised to see the use of an explicit thread for this when we were using async/await for everything since I might have naively thought that the runtime would manage threads for us and we could specify through our use of .await where we wanted to block.

But it's possible I'm not thinking about this right, these are just initial thoughts.

ebuchman

Great work. Let's merge this and open a follow up issue to track some fixes (eg adding pings) and any future work (refactor of APIs or internals)

Addressed or to be dealt with in follow ups

romac · 2020-09-17T11:00:52Z

Great stuff @thanethomson! 💯

thanethomson added 7 commits August 10, 2020 18:24

Create basic request/response transport abstraction

9df13ed

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add fixture-based transport with failing test

a01883d

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Refactor mock transport

a03d0ab

This version of the transport provides a request matching interface, along with an implementation of a request method-based matcher (produces specific fixed responses according to the request's method). Signed-off-by: Thane Thomson <connect@thanethomson.com>

Take client creation interface change into account

73c2626

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Reword docstring for Transport

8650db1

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Ignore integration test

20cce0a

Signed-off-by: Thane Thomson <connect@thanethomson.com>

xla changed the title ~~rpc: Add event subscription management for RPC client~~ rpc: event subscription management for RPC client Aug 12, 2020

xla suggested changes Aug 12, 2020

View reviewed changes

thanethomson and others added 6 commits August 12, 2020 11:52

Update rpc/Cargo.toml

92cae5f

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

Update rpc/src/client.rs

40e7de2

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

Update rpc/src/client/subscription.rs

4634e48

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

Update rpc/src/client/subscription.rs

82cf571

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

Update rpc/src/client/subscription.rs

52c36d0

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

Update rpc/src/client/transport.rs

1089e8d

Co-authored-by: Alexander Simmerl <a.simmerl@gmail.com>

thanethomson added 13 commits August 12, 2020 12:02

Simplify submodule naming

5994999

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Reorder optional dependencies alphabetically

1ef2305

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Rename module to singular

a364187

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Destructure and reformat imports

15d5765

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add note for myself on TODO

da0247a

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Move mod declarations before use statements

cafa8d7

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix clippy warnings

9ad109b

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Reduce number of blocks grabbed to speed up test

cf56f7b

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Improve documentation for RPC client

ae66f90

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix links in method docs

d3d2c32

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Expose client docs in base RPC package

f01d13d

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson mentioned this pull request Aug 27, 2020

rpc: ADR for client event subscription mechanism #476

Merged

5 tasks

Clarify docs for Subscription::terminate

99a321f

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Merge branch 'master' into rpc/subscription

3e38b0b

Merges the latest changes from `master` while adjusting for Tendermint v0.34 compatibility. Signed-off-by: Thane Thomson <connect@thanethomson.com>

melekes reviewed Sep 15, 2020

View reviewed changes

rpc/src/client/subscription.rs Outdated Show resolved Hide resolved

thanethomson and others added 4 commits September 15, 2020 08:39

Fix features for tendermint-rpc dependency in light client

58d52ef

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Fix clippy warnings

e218dd6

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Do not feature-guard IoError::IoError variant

ee70331

Rename IoError::IoError to IoError::RpcError

e7bb6a1

Refactor subscription state modelling

a891c21

Signed-off-by: Thane Thomson <connect@thanethomson.com>

ebuchman reviewed Sep 15, 2020

View reviewed changes

thanethomson added 5 commits September 15, 2020 14:02

Replace SubscriptionId AsRef implementation with as_str() method for …

c22a5da

…clarity Signed-off-by: Thane Thomson <connect@thanethomson.com>

Rename TxResult and TxResultResult for clarity

1753566

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add explanation of SubscriptionRouter subscriptions field

4db8d9e

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Comment on assumption regarding handling of event publishing failure

e293693

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add comment explaining redeclaration and obtaining of local var

e7403a6

Signed-off-by: Thane Thomson <connect@thanethomson.com>

ebuchman reviewed Sep 15, 2020

View reviewed changes

ebuchman approved these changes Sep 16, 2020

View reviewed changes

ebuchman merged commit 64d29c9 into master Sep 16, 2020

ebuchman deleted the rpc/subscription branch September 16, 2020 18:00

thanethomson mentioned this pull request Sep 16, 2020

rpc: Rename WebSocketSubscriptionClient #566

Merged

5 tasks

This was referenced Sep 16, 2020

event_listener Web socket error #311

Closed

WebSockets follow up #568

Closed

thanethomson mentioned this pull request Sep 17, 2020

rpc: Simplify client interface #569

Merged

5 tasks

ebuchman mentioned this pull request Sep 29, 2020

Websocket Pings #590

Closed

Flag	Description	Additional Dependencies
`default`	Just the RPC types	None
`client`	All the client-related traits, but no implementations	`async-trait`, `futures`, `tokio`
`http`	`HttpClient` (implements `Client`)	`http`, `hyper`, `tokio`
`websocket`	`WebSocketSubscriptionClient`, and in future a `WebSocketClient`, which will implement the `Client` and `SubscriptionClient` traits and will supersede the `WebSocketSubscriptionClient`	`async-tungstenite`, `tokio`
`mock`	`MockClient` and `MockSubscriptionClient`	`tokio`

		@@ -1,216 +0,0 @@
		//! Tendermint Websocket event listener client

rpc: event subscription management for RPC client #516

rpc: event subscription management for RPC client #516

Conversation

thanethomson commented Aug 12, 2020 • edited Loading

xla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thanethomson commented Aug 12, 2020

thanethomson commented Aug 14, 2020

romac commented Sep 15, 2020

thanethomson commented Sep 15, 2020

ebuchman 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

thanethomson Sep 15, 2020 • 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

ebuchman Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac commented Sep 15, 2020 • edited Loading

ebuchman 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

ebuchman left a comment

Choose a reason for hiding this comment

romac commented Sep 17, 2020

thanethomson commented Aug 12, 2020 •

edited

Loading

thanethomson Sep 15, 2020 •

edited

Loading

ebuchman Sep 15, 2020 •

edited

Loading

romac commented Sep 15, 2020 •

edited

Loading