R4R: Dynamic Capabilities with Routing #5888

AdityaSripal · 2020-03-28T03:20:17Z

Closes: #5542

Description

Start routing implementation in new PR since it may require changes to design. Currently router from portID => module callbacks is in portkeeper

All routing gets handled within the IBC handler itself

For contributor use:

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer

For admin use:

Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

x/ibc/04-channel/types/msgs.go

x/ibc/05-port/keeper/keeper.go

x/ibc/05-port/types/module.go

AdityaSripal · 2020-03-28T03:22:07Z

x/ibc/05-port/types/module.go

+		packet channeltypes.Packet,
+	) (*sdk.Result, error)
+
+	GetCapability(ctx sdk.Context, name string) (capability.Capability, bool)


Put Get/Claim capability callbacks as well so that ibc can request a module to provide the right capability or pass a module a capability that it can claim

Hmm, I'm a bit confused by these - why would IBC need to request a capability from a module or vice-versa? Generally in an object-capability system it is very dangerous for a module to expose an API that allows other modules to just fetch capabilities.

AdityaSripal · 2020-03-28T03:22:32Z

x/ibc/20-transfer/handler.go

@@ -13,20 +13,6 @@ func NewHandler(k Keeper) sdk.Handler {
 		switch msg := msg.(type) {
 		case MsgTransfer:
 			return handleMsgTransfer(ctx, k, msg)
-		case channeltypes.MsgPacket:


No longer handled in 20-transfer's Handler

AdityaSripal · 2020-03-28T03:27:38Z

x/ibc/20-transfer/module.go

+	}
+}
+
+func (am AppModule) OnAcknowledgementPacket(


Currently Acknowledgement callback doesn't exist, so i have a no-op. Will implement spec later

AdityaSripal · 2020-03-28T03:28:27Z

x/ibc/handler.go

@@ -37,22 +38,75 @@ func NewHandler(k Keeper) sdk.Handler {

 		// IBC channel msgs
 		case channel.MsgChannelOpenInit:
-			res, cap, err := channel.HandleMsgChannelOpenInit(ctx, k.ChannelKeeper, msg)
+			cbs := k.PortKeeper.LookupModule(ctx, msg.PortID)
+			portCap, _ := cbs.GetCapability(ctx, ibctypes.PortPath(msg.PortID))


Get port capability from module using callback

Why do we need to look this up? The module should already have the port capability.

AdityaSripal · 2020-03-28T03:28:46Z

x/ibc/handler.go

-			res, cap, err := channel.HandleMsgChannelOpenInit(ctx, k.ChannelKeeper, msg)
+			cbs := k.PortKeeper.LookupModule(ctx, msg.PortID)
+			portCap, _ := cbs.GetCapability(ctx, ibctypes.PortPath(msg.PortID))
+			err := cbs.OnChanOpenInit(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortID, msg.ChannelID, msg.Channel.Counterparty, msg.Channel.Version)


Call user defined OnChanOpenInit callback

Hmm, should this happen before or after HandleMsgChannelOpenInit?

Both are safe as long as the transaction is atomic. If it's after then it can expect the channel state to be there.

AdityaSripal · 2020-03-28T03:29:06Z

x/ibc/handler.go

+				return nil, err
+			}
+			res, cap, err := channel.HandleMsgChannelOpenInit(ctx, k.ChannelKeeper, portCap, msg)
+			cbs.ClaimCapability(ctx, cap, ibctypes.ChannelCapabilityPath(msg.PortID, msg.ChannelID))


Module claims created channel capability using callback

Instead of doing it this way, how about passing the channel capability to OnChanOpenInit ? I think that's cleaner.

AdityaSripal · 2020-03-28T03:29:44Z

x/ibc/handler.go

+
+		// IBC packet msgs get routed to the appropriate module callback
+		case channel.MsgPacket:
+			cbs := k.PortKeeper.LookupModule(ctx, msg.Packet.DestinationPort)


Rather than assuming portID == moduleName and using baseapp Router, we lookup module here in IBC handler and call appropriate callback

This actually needs to be based on the channel capability, not the port capability

cwgoes

Unfortunately I don't think we can safely use a map in this way - the routing table needs to be persisted in an analogous way to how dynamic capabilities are - perhaps the moduleName on scoped capability keepers can be used to help us out here? When an incoming packet comes in, for example, we should look up (in the capability ownership table) which module owns the channel capability associated with that packet, then call the callback on that module. Since the module set is fixed in app.go in all SDK apps, it's fine to have a static map of moduleName to callbacks.

x/ibc/05-port/keeper/keeper.go

cwgoes · 2020-03-30T07:25:47Z

x/ibc/05-port/keeper/keeper.go

@@ -45,6 +47,8 @@ func (k *Keeper) BindPort(ctx sdk.Context, portID string) capability.Capability
 		panic(err.Error())
 	}

+	k.router[portID] = cbs


The problem is that BindPort will not be re-called when e.g. gaiad is stopped & re-started

Can we delete this now?

cwgoes · 2020-03-30T07:26:43Z

x/ibc/05-port/types/module.go

+		packet channeltypes.Packet,
+	) (*sdk.Result, error)
+
+	GetCapability(ctx sdk.Context, name string) (capability.Capability, bool)


Hmm, I'm a bit confused by these - why would IBC need to request a capability from a module or vice-versa? Generally in an object-capability system it is very dangerous for a module to expose an API that allows other modules to just fetch capabilities.

cwgoes · 2020-03-30T07:27:11Z

x/ibc/20-transfer/module.go

+	counterparty channeltypes.Counterparty,
+	version string,
+) error {
+	// TODO: Enforce ordering, currently relayers use ORDERED channels


In principle, token transfer channels can safely be unordered. I think we should test the relayer with both.

cwgoes · 2020-03-30T07:27:34Z

x/ibc/20-transfer/module.go

+) error {
+	// TODO: Enforce ordering, currently relayers use ORDERED channels
+
+	if counterparty.PortID != types.PortID {


Not necessarily, it's not "safety-critical", just convention. I think it's useful convention for now though.

cwgoes · 2020-03-30T07:29:01Z

x/ibc/handler.go

@@ -37,22 +38,75 @@ func NewHandler(k Keeper) sdk.Handler {

 		// IBC channel msgs
 		case channel.MsgChannelOpenInit:
-			res, cap, err := channel.HandleMsgChannelOpenInit(ctx, k.ChannelKeeper, msg)
+			cbs := k.PortKeeper.LookupModule(ctx, msg.PortID)
+			portCap, _ := cbs.GetCapability(ctx, ibctypes.PortPath(msg.PortID))


Why do we need to look this up? The module should already have the port capability.

cwgoes · 2020-03-30T07:29:50Z

x/ibc/handler.go

-			res, cap, err := channel.HandleMsgChannelOpenInit(ctx, k.ChannelKeeper, msg)
+			cbs := k.PortKeeper.LookupModule(ctx, msg.PortID)
+			portCap, _ := cbs.GetCapability(ctx, ibctypes.PortPath(msg.PortID))
+			err := cbs.OnChanOpenInit(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortID, msg.ChannelID, msg.Channel.Counterparty, msg.Channel.Version)


Hmm, should this happen before or after HandleMsgChannelOpenInit?

Both are safe as long as the transaction is atomic. If it's after then it can expect the channel state to be there.

cwgoes · 2020-03-30T07:30:48Z

x/ibc/handler.go

+				return nil, err
+			}
+			res, cap, err := channel.HandleMsgChannelOpenInit(ctx, k.ChannelKeeper, portCap, msg)
+			cbs.ClaimCapability(ctx, cap, ibctypes.ChannelCapabilityPath(msg.PortID, msg.ChannelID))


Instead of doing it this way, how about passing the channel capability to OnChanOpenInit ? I think that's cleaner.

cwgoes · 2020-03-30T07:32:41Z

x/ibc/handler.go

+
+		// IBC packet msgs get routed to the appropriate module callback
+		case channel.MsgPacket:
+			cbs := k.PortKeeper.LookupModule(ctx, msg.Packet.DestinationPort)


This actually needs to be based on the channel capability, not the port capability

cwgoes · 2020-03-30T18:39:03Z

Basic pattern

Static map based on module name from scoped capability keeper to callbacks
When a message comes in for ConnOpenInit (analogously for ConnOpenTry)
- Message has a port
- Look up which module owns that port (for now: first module)
  - Needs new function on ScopedCapabilityKeeper to look up first other owning module
- Find (in the capability mapping) the moduleName for that module
- Use the static map to look up the callbacks
- Call onConnOpenInit and pass the new channel capability as an argument (module must store)
For last two steps of handshake & packets, route based on channel capability instead

Notes

Routing has to exclude the IBC module's moduleName

cwgoes

Almost there! A few minor concerns / cleanup remaining.

cwgoes · 2020-03-31T12:49:57Z

x/ibc/04-channel/keeper/keeper.go

+	}
+	// the module that owns the channel, for now, is the first module that isn't "ibc"
+	// that owns the module
+	module := capOwners.Owners[1].Module


Are we sure that the ordering will always be this way? Isn't it dependent on the module name?

yeah this needs a test case for sure

cwgoes · 2020-03-31T12:50:27Z

x/ibc/05-port/keeper/keeper.go

@@ -45,6 +47,8 @@ func (k *Keeper) BindPort(ctx sdk.Context, portID string) capability.Capability
 		panic(err.Error())
 	}

+	k.router[portID] = cbs


Can we delete this now?

cwgoes · 2020-03-31T12:50:40Z

x/ibc/05-port/keeper/keeper.go

+	}
+	// the module that owns the port, for now, is the first module that isn't "ibc"
+	// that owns the module
+	module := capOwners.Owners[1].Module


Are we sure that the ordering will always be this way? Isn't it dependent on the module name?

ditto test case

cwgoes · 2020-03-31T12:51:23Z

x/ibc/05-port/types/module.go

+		connectionHops []string,
+		portID,
+		channelID string,
+		portCap capability.Capability,


We shouldn't need to pass the port capability to the module

(we do need to pass the channel capability)

whoops misnamed

x/ibc/05-port/keeper/keeper.go

fedekunze

good work @AdityaSripal! Thanks for tackling this

x/ibc/04-channel/keeper/keeper.go

fedekunze · 2020-03-31T17:48:55Z

x/ibc/04-channel/keeper/keeper.go

+	}
+	// the module that owns the channel, for now, is the first module that isn't "ibc"
+	// that owns the module
+	module := capOwners.Owners[1].Module


yeah this needs a test case for sure

x/ibc/05-port/keeper/keeper.go

fedekunze · 2020-03-31T17:50:51Z

x/ibc/05-port/keeper/keeper.go

+	}
+	// the module that owns the port, for now, is the first module that isn't "ibc"
+	// that owns the module
+	module := capOwners.Owners[1].Module


ditto test case

x/ibc/05-port/types/module.go

fedekunze · 2020-03-31T17:58:54Z

x/ibc/handler.go

+			// Lookup module by port capability
+			module, portCap, ok := k.PortKeeper.LookupModuleByPort(ctx, msg.PortID)
+			if !ok {
+				return nil, sdkerrors.Wrap(porttypes.ErrInvalidPort, "could not retrieve module from portID")


Suggested change

return nil, sdkerrors.Wrap(porttypes.ErrInvalidPort, "could not retrieve module from portID")

return nil, sdkerrors.Wrapf(porttypes.ErrInvalidPort, "could not retrieve module from portID %s" msg.PortID)

fedekunze · 2020-03-31T17:59:46Z

x/ibc/handler.go

+			// Lookup module by port capability
+			module, portCap, ok := k.PortKeeper.LookupModuleByPort(ctx, msg.PortID)
+			if !ok {
+				return nil, sdkerrors.Wrap(porttypes.ErrInvalidPort, "could not retrieve module from portID")


Suggested change

return nil, sdkerrors.Wrap(porttypes.ErrInvalidPort, "could not retrieve module from portID")

return nil, sdkerrors.Wrapf(porttypes.ErrInvalidPort, "could not retrieve module from portID %s" msg.PortID)

x/ibc/handler.go

lgtm-com · 2020-03-31T18:08:40Z

This pull request fixes 2 alerts when merging 8086e42 into 06e7b80 - view on LGTM.com

fixed alerts:

2 for Useless assignment to local variable

fedekunze

LGTM. minor suggestion

fedekunze · 2020-03-31T20:22:15Z

x/ibc/04-channel/keeper/keeper.go

+	capOwners, ok := k.scopedKeeper.GetOwners(ctx, ibctypes.ChannelCapabilityPath(portID, channelID))
+	if !ok {
+		return "", nil, false
+	}
+	if len(capOwners.Owners) < 2 {
+		panic("more than one module has bound to this port; currently not supported")
+	}
+	// the module that owns the channel, for now, is the only module owner that isn't "ibc"
+	var module string
+	if capOwners.Owners[0].Module == "ibc" {
+		module = capOwners.Owners[1].Module
+	} else {
+		module = capOwners.Owners[0].Module
+	}
+
+	cap, found := k.scopedKeeper.GetCapability(ctx, ibctypes.ChannelCapabilityPath(portID, channelID))
+	if !found {
+		return "", nil, false
+	}
+	return module, cap, true


nit, you can avoid duplication by abstracting this part and passing the path:

func (k ScopedKeeper) LookupModule(ctx sdk.Context, path string) (string, capability.Capability, bool) { capOwners, ok := k.GetOwners(ctx, path) if !ok { return "", nil, false } // For now, we enforce that only IBC and the module bound to port can own the capability // while future implementations may allow multiple modules to bind to a port, currently we // only allow one module to be bound to a port at any given time if len(capOwners.Owners) != 2 { panic("more than one module has bound to this port; currently not supported") } // the module that owns the port, for now, is the only module owner that isn't "ibc" var module string if capOwners.Owners[0].Module == "ibc" { module = capOwners.Owners[1].Module } else { module = capOwners.Owners[0].Module } cap, found := k.GetCapability(ctx, path) if !found { return "", nil, false } return module, cap, true }

and then:

// LookupModuleByChannel will return the IBCModule along with the capability associated with a given channel defined by its portID and channelID func (k Keeper) LookupModuleByChannel(ctx sdk.Context, portID, channelID string) (string, capability.Capability, bool) { return k.scopedKeeper.LookupModule(ctx, ibctypes.ChannelCapabilityPath(portID, channelID)) }

Hmm yea doesn't make sense to have the ibc special case in the scopedKeeper code, but I can just return string of modules from scoped keeper and then have a util function in ibc to enforce length-2 and pick non-ibc module

lgtm-com · 2020-04-01T22:49:00Z

This pull request fixes 3 alerts when merging b282f99 into 06e7b80 - view on LGTM.com

fixed alerts:

3 for Useless assignment to local variable

golangcibot · 2020-04-02T19:45:28Z

crypto/keyring/keyring_test.go

+	algo := Secp256k1
+
+	n1, n2, n3 := "some dude", "a dudette", "dude-ish"
+	p1, p2, p3 := "1234", "foobar", "foobar"


string foobar has 3 occurrences, but such constant foobar already exists (from goconst)

golangcibot · 2020-04-02T19:45:29Z

crypto/keyring/keyring_test.go

+
+	algo := Secp256k1
+	n1, n2, n3 := "personal", "business", "other"
+	p1, p2 := "1234", "really-secure!@#$"


string 1234 has 5 occurrences, but such constant nums already exists (from goconst)

golangcibot · 2020-04-02T19:45:29Z

x/ibc/03-connection/keeper/keeper.go


-		if cb(connection) {
+		if cb(types.IdentifiedConnectionEnd{connection, identifier}) {


composites: github.com/cosmos/cosmos-sdk/x/ibc/03-connection/types.IdentifiedConnectionEnd composite literal uses unkeyed fields (from govet)

lgtm-com · 2020-04-02T19:52:11Z

This pull request fixes 3 alerts when merging 9b1aeda into 06e7b80 - view on LGTM.com

fixed alerts:

3 for Useless assignment to local variable

AdityaSripal · 2020-04-02T20:22:37Z

client/keys/testdata/keys/keys.db/CURRENT

@@ -0,0 +1 @@
+MANIFEST-000004


Seems like some test files got committed accidentally. Is there a command to clean these files up or should i manually delete them?

@alexanderbez

errr, not sure how this happened. These files exist on master due to @alessio's changes to the keybase. But I branched off of ibc-alpha, so not sure how these got in here. They also were not in my PR diff.

AdityaSripal · 2020-04-02T20:23:53Z

client/keys/testdata/keys/keys.db/CURRENT

@@ -0,0 +1 @@
+MANIFEST-000004


Seems like some test files got committed accidentally. Is there a command to clean these files up or should i manually delete them?

@alexanderbez

cwgoes

Logic looks solid, seems like a few test-cases can be uncommented, a few nits.

x/ibc/04-channel/keeper/handshake_test.go

x/ibc/05-port/keeper/keeper.go

x/ibc/05-port/types/module.go

cwgoes · 2020-04-03T07:41:50Z

x/ibc/05-port/types/router.go

+// GetRoute returns a IBCModule for a given module.
+func (rtr *Router) GetRoute(module string) IBCModule {
+	if !rtr.HasRoute(module) {
+		panic(fmt.Sprintf("route does not exist for module %s", module))


This seems potentially dangerous? What if a packet has a port/channel that isn't routed?

x/ibc/20-transfer/keeper/keeper.go

cwgoes · 2020-04-03T07:43:18Z

x/ibc/20-transfer/module.go

+		return sdkerrors.Wrap(channel.ErrChannelCapabilityNotFound, err.Error())
+	}
+
+	// TODO: escrow


Here is where we want to create the escrow address

Looks like the current implementation just calculates the escrow address from portID, and channelID and uses the bank keeper to send and receive money. This seems like it should work?

Should I make these escrow addresses module accounts instead?

https://github.com/cosmos/cosmos-sdk/blob/ibc-alpha/x/ibc/20-transfer/keeper/relay.go#L97

https://github.com/cosmos/cosmos-sdk/blob/ibc-alpha/x/ibc/20-transfer/keeper/relay.go#L186

This is OK for now, I suppose, maybe they should be module accounts but that can be a follow-up.

cwgoes · 2020-04-03T07:43:21Z

x/ibc/20-transfer/module.go

+		return sdkerrors.Wrap(channel.ErrChannelCapabilityNotFound, err.Error())
+	}
+
+	// TODO: escrow


Here is where we want to create the escrow address

Co-Authored-By: Christopher Goes <cwgoes@pluranimity.org>

…-sdk into aditya/ibc-routing

x/ibc/types/utils.go

cwgoes

utACK modulo question on ClaimCapability safety / permissions

fedekunze · 2020-04-06T20:04:16Z

NOTE: Capability is no longer an interface but a concrete type

fedekunze · 2020-04-06T22:33:21Z

x/ibc/handler.go

+			// Lookup module by port capability
+			module, portCap, ok := k.PortKeeper.LookupModuleByPort(ctx, msg.PortID)
+			if !ok {
+				return nil, sdkerrors.Wrap(port.ErrInvalidPort, "could not retrieve module from portID")
+			}
+			res, cap, err := channel.HandleMsgChannelOpenInit(ctx, k.ChannelKeeper, portCap, msg)
+			if err != nil {
+				return nil, err
+			}
+			// Retrieve callbacks from router
+			cbs, ok := k.Router.GetRoute(module)
+			if !ok {
+				return nil, sdkerrors.Wrapf(port.ErrInvalidRoute, "route not found to module: %s", module)
+			}
+			err = cbs.OnChanOpenInit(ctx, msg.Channel.Ordering, msg.Channel.ConnectionHops, msg.PortID, msg.ChannelID, cap, msg.Channel.Counterparty, msg.Channel.Version)
+			if err != nil {
+				return nil, err
+			}
+
+			return res, nil


nit, can we move each of the cases to a private function to avoid bloating the switch and for ease of reading?

AdityaSripal added 8 commits March 25, 2020 22:54

start integrating capabilities

57aacb4

switch to using port authenticate

1ed4923

fix build and address first review

8ef8225

remove capabilities from msgs and return them on channel handlers

06e7b80

implement channel callbacks and capability injection

7e01aac

route packets through ibc handler and then to module

5979b3b

start implementing transfer with new routing logic

bddc7a9

fix packet msg routes

18f7207

AdityaSripal requested review from alessio, alexanderbez, fedekunze, hschoenburg and nylira as code owners March 28, 2020 03:20

AdityaSripal requested review from cwgoes and mossid March 28, 2020 03:20

AdityaSripal commented Mar 28, 2020

View reviewed changes

cwgoes suggested changes Mar 30, 2020

View reviewed changes

implement routing from @cwgoes comment

ff0dbef

cwgoes suggested changes Mar 31, 2020

View reviewed changes

fedekunze added x/ibc labels Mar 31, 2020

address @cwgoes comments

8086e42

fedekunze suggested changes Mar 31, 2020

View reviewed changes

address @fedekunze comments

56a4c9c

fedekunze approved these changes Mar 31, 2020

View reviewed changes

start working on tests, prove GetCapability bug

b282f99

fix channel tests

287bd62

update to latest master

9b1aeda

golangcibot reviewed Apr 2, 2020

View reviewed changes

AdityaSripal commented Apr 2, 2020

View reviewed changes

AdityaSripal changed the base branch from aditya/ics5 to ibc-alpha April 2, 2020 20:24

AdityaSripal changed the title ~~WIP: IBC Routing~~ Dynamic Capabilities with Routing Apr 2, 2020

AdityaSripal changed the title ~~Dynamic Capabilities with Routing~~ R4R: Dynamic Capabilities with Routing Apr 2, 2020

cwgoes suggested changes Apr 3, 2020

View reviewed changes

cwgoes mentioned this pull request Apr 3, 2020

Acknowledgements in ICS 20 #5923

Closed

AdityaSripal and others added 4 commits April 3, 2020 10:58

Update x/ibc/05-port/types/module.go

d23c401

Co-Authored-By: Christopher Goes <cwgoes@pluranimity.org>

test capability claiming and don't panic on invalid route

7eb7c51

Merge branch 'aditya/ibc-routing' of https://github.com/cosmos/cosmos…

056d759

…-sdk into aditya/ibc-routing

deduplicate

dca2a0b

golangcibot reviewed Apr 3, 2020

View reviewed changes

x/ibc/types/utils.go Outdated Show resolved Hide resolved

AdityaSripal added 3 commits April 3, 2020 18:36

Require sending capability to SendPacket

0e1775b

appease linter

a6cfc6a

fix conflicts

d08a246

cwgoes approved these changes Apr 6, 2020

View reviewed changes

cwgoes mentioned this pull request Apr 6, 2020

ICS5: Integrate Dynamic Capabilities into IBC #5872

Closed

11 tasks

cwgoes added R4R and removed WIP labels Apr 6, 2020

fix conflicts, build, tests

b92d5f7

fedekunze reviewed Apr 6, 2020

View reviewed changes

cwgoes merged commit 9448610 into ibc-alpha Apr 7, 2020

cwgoes deleted the aditya/ibc-routing branch April 7, 2020 11:20

This was referenced Apr 7, 2020

Dynamic capability & routing implementation #5542

Closed

Implement additional callbacks as per ICS 26 #5507

Closed

AdityaSripal mentioned this pull request Apr 8, 2020

Bind Transfer Port on InitChain #5954

Merged

11 tasks

	return nil, sdkerrors.Wrap(porttypes.ErrInvalidPort, "could not retrieve module from portID")
	return nil, sdkerrors.Wrapf(porttypes.ErrInvalidPort, "could not retrieve module from portID %s" msg.PortID)


		if cb(connection) {
		if cb(types.IdentifiedConnectionEnd{connection, identifier}) {

		@@ -0,0 +1 @@
		MANIFEST-000004

		@@ -0,0 +1 @@
		MANIFEST-000004

R4R: Dynamic Capabilities with Routing #5888

R4R: Dynamic Capabilities with Routing #5888

Conversation

AdityaSripal commented Mar 28, 2020

Description

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

cwgoes 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

cwgoes commented Mar 30, 2020 • edited Loading

cwgoes 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

fedekunze 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

lgtm-com bot commented Mar 31, 2020

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 2, 2020

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

cwgoes 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

cwgoes left a comment

Choose a reason for hiding this comment

fedekunze commented Apr 6, 2020

Choose a reason for hiding this comment

cwgoes commented Mar 30, 2020 •

edited

Loading