Enable Fee Delegation #5207

ethanfrey · 2019-10-16T14:13:35Z

Targeted PR against correct branch (see CONTRIBUTING.md)

This PR is a complete rewrite of #4616 as cosmos-sdk is evolving quickly.
This was an approach accepted by the core team, and refined in https://gist.github.com/aaronc/b60628017352df5983791cad30babe56
Or course, now I can use modular ante handlers to do so more cleanly.

Copied description from #4616

Use cases:

an organization can delegate fees to employees from the master organizational account so that employees can sign certain transactions onto the blockchain without needing to worry about wallets
a user could keep a less trusted key on their phone and delegate fees to this key for purposes such as voting on proposals (this would also use generic action delegation which was worked on in the hackathon and will be submitted as a separate PR)
a third party service could delegate fees to users who pay off-chain via fiat or other crypto so that those users can use Dapps without needing to fill their wallets

Current state:

All internal logic, keeper, handler and even a custom ante handler are implemented. As well as integration into SimApp. What is left:

Genesis file logic
PeriodicFeeAllowance implementation

Client (cli and rest) will be done in another PR, as integrating a non-StdTx type into the framework will be less than trivial.

However, none of this is core to the design, I have a few design questions that can easily be reviewed and answered at the current state, and I would like to submit this for review:

Is changing StdTx acceptable? Should I just modify StdFee instead? Do you have some other idea that is less intrusive?
Does x/delegation/internal/ante package look good - as you intended the modular ante handlers to be used
I note that the chained decorators don't allow submitting a tx from an empty account (no funds). However, that is expected to be a common case for delegated fees (I delegate to a temporary account so it can take some actions without needing tokens). I can change the stack in delegation.NewAnteHandler to remove that restriction, but I wanted to hear the reasoning first to make sure I don't break anything else (note the TODO and comments in internal/ante/fee_test.go

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
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, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2019-10-16T14:25:19Z

Codecov Report

Merging #5207 into master will decrease coverage by 0.01%.
The diff coverage is 53.93%.

@@            Coverage Diff             @@
##           master    #5207      +/-   ##
==========================================
- Coverage   54.61%   54.59%   -0.02%     
==========================================
  Files         299      310      +11     
  Lines       18177    18621     +444     
==========================================
+ Hits         9928    10167     +239     
- Misses       7464     7650     +186     
- Partials      785      804      +19

x/delegation/internal/types/key.go

x/delegation/internal/keeper/keeper.go

x/delegation/internal/types/expiration.go

x/delegation/internal/types/basic_fee_test.go

x/delegation/handler.go

…ee-delegation-new

jackzampolin · 2019-10-16T20:24:32Z

x/auth/types/stdsignmsg.go

@@ -8,15 +8,16 @@ import (
 // a Msg with the other requirements for a StdSignDoc before
 // it is signed. For use in the CLI.
 type StdSignMsg struct {


Note: This PR changes the StdSignMsg. Is there any way to avoid this or make it more general than just a FeeAccount? What other types of delegations?

There is no other way to add this info without changing the sign bytes.
The info is tx global (not msg specific), and thus needs to be there.

We can also stop using any concrete structs and use only interfaces, and architect the entire runtTx, anteHandler in a way that we use reflection to detect what kind of Tx it is, but that is a much deeper change. Eg. we never check StdSignMsg, StdTx, etc as arguments, just abstract interfaces.

Then add a DelegatedSignMsg with this extra FeeAccount field and a GetFeeAccount() method. The AnteHandler can use reflection to see if the implementation has this method or not. And will allow various tx types coming in.

That said, this also seems dangerous to allow modules to register new tx types, with possilbly different sign byte calculations, etc. And would require a deep ADR approach.

Looking more into the code, I could easily just modify StdFee to have one more field for the payer, which would probably be cleaner. I just followed the approach Aaron had done as I believed it had been discussed with the sdk team.

Modifying StdFee, even with an optional field, will change StdTx, however it will still accept the old format without any changes, and this will be a lot less breaking to many of the interfaces. If you prefer that, I can update the code.

I think that is also much less invasive than try to abstract out the field in some optional interface and have multiple different Tx types that are switched between at runtime.

x/feegrant/alias.go

fedekunze

utACK. Thank you @ethanfrey

Only events missing and simplify the tests by using the Simapp.

x/feegrant/handler.go

fedekunze · 2019-11-07T14:57:19Z

x/feegrant/internal/keeper/keeper_test.go

+	suite.Run(t, new(KeeperTestSuite))
+}
+
+func (suite *KeeperTestSuite) SetupTest() {


we can use the simapp to simplify the setup. Ref:

cosmos-sdk/x/evidence/internal/keeper/keeper_test.go

Lines 26 to 46 in 95ddc24

func (suite *KeeperTestSuite) SetupTest() {

checkTx := false

app := simapp.Setup(checkTx)

// get the app's codec and register custom testing types

cdc := app.Codec()

cdc.RegisterConcrete(types.TestEquivocationEvidence{}, "test/TestEquivocationEvidence", nil)

// recreate keeper in order to use custom testing types

evidenceKeeper := evidence.NewKeeper(

cdc, app.GetKey(evidence.StoreKey), app.GetSubspace(evidence.ModuleName),

evidence.DefaultCodespace,

)

router := evidence.NewRouter()

router = router.AddRoute(types.TestEvidenceRouteEquivocation, types.TestEquivocationHandler(*evidenceKeeper))

evidenceKeeper.SetRouter(router)

suite.ctx = app.BaseApp.NewContext(checkTx, abci.Header{Height: 1})

suite.querier = keeper.NewQuerier(*evidenceKeeper)

suite.keeper = *evidenceKeeper

}

alexanderbez

Performed another pass-through. I noticed this has no spec/. This PR needs that to, at the very least, outline all the events returned.

In addition, an accompanying PR should be made in Gaia to use this new module along with the new ante-handler chain. This isn't blocking this PR, but I'd at least like to see a draft of the PR before this is merged.

x/feegrant/handler.go

x/feegrant/internal/types/codec.go

alexanderbez · 2019-11-07T16:20:28Z

x/feegrant/internal/types/errors.go

+
+var (
+	// ErrFeeLimitExceeded error if there are not enough allowance to cover the fees
+	ErrFeeLimitExceeded = sdkerrors.Register(DefaultCodespace, 1, "fee limit exceeded")


This is just a question. Is this the defacto way of custom/module-level error implementation? In the evidence module I simply use the old paradigm of a function wrapper to return sdkerrors.New(...).

This should be the de-facto reference I believe: https://github.com/cosmos/cosmos-sdk/blob/master/types/errors/errors.go#L16

Register ensures there are never two errors registered with the same codespace and code

alexanderbez · 2019-11-07T16:22:03Z

x/feegrant/internal/types/key.go

+
+// FeeAllowanceKey is the canonical key to store a grant from granter to grantee
+// We store by grantee first to allow searching by everyone who granted to you
+func FeeAllowanceKey(granter sdk.AccAddress, grantee sdk.AccAddress) []byte {


see previous note in the keeper file.

I cannot see this note in the review - there are so many notes they all get collapsed by the webui. Can you please reference the note (link or just cut-paste the comment)?

alexanderbez · 2019-11-07T16:23:42Z

x/feegrant/internal/keeper/keeper.go

+// GetFeeGrant returns entire grant between both accounts
+func (k Keeper) GetFeeGrant(ctx sdk.Context, granter sdk.AccAddress, grantee sdk.AccAddress) (types.FeeAllowanceGrant, bool) {
+	store := ctx.KVStore(k.storeKey)
+	key := types.FeeAllowanceKey(granter, grantee)


ref: #5254

We should ideally be using prefix.NewStore. This is the design approach going forward with new modules. See the evidence module for guidance.

Yes. It is very hard to pass a review with a moving target and new standards being added between every review pass. This seemed to be standard when I started the PR. I can update.

alexanderbez · 2019-11-07T16:29:03Z

x/feegrant/internal/types/tx.go

+// adding the ability to delegate the fee payment
+// NOTE: the first signature responsible for paying fees, either directly,
+// or must be authorized to spend from the provided Fee.FeeAccount
+type FeeGrantTx struct {


question/non-blocking: Is there absolutely no way for us to utilize the existing StdTx type (it's ok to change the Fee type)?

This was your idea: #5207 (comment)

I proposed to make the change to StdTx/StdFee

(Actually I implemented until you and Aditya decided it was better this way. I actually found it much simpler adding the fields to StdTx)

So how should we proceed on this @alexanderbez? We can either keep the custom FeeGrantTx or just add a FeeAccount sdk.AccAddress field to StdTx as was the original design.

Upon reflecting on this, my thought is that if this is in the SDK and going onto cosmos-hub, then to me it makes sense this would be part of StdTx. If this were only going on a zone like Regen, we should have a custom tx

alexanderbez · 2019-11-07T16:29:31Z

x/feegrant/module.go

+// DefaultGenesis returns default genesis state as raw bytes for the feegrant
+// module.
+func (AppModuleBasic) DefaultGenesis() json.RawMessage {
+	return []byte("[]")


Suggested change

return []byte("[]")

return []byte("{}")

You made this comment before and I responded:
#5207 (comment)

ethanfrey

?

ethanfrey · 2019-11-07T17:24:00Z

I don't see a clear path to ever finish this PR.

I would be happy to hand this off to the core-sdk team. You can make a new branch and perform all modifications as you wish. This should much faster for all of us. I assume this was a desired feature in gaia, and I guess you have strong ideas on how it should work as you maintain gaia, which is fine. Who better to ensure it is all to your code standards than the core developers.

(You actually should have the power to make edits directly on this branch as well already)

alexanderbez · 2019-11-07T19:40:56Z

I don't see a clear path to ever finish this PR.

I would be happy to hand this off to the core-sdk team. You can make a new branch and perform all modifications as you wish. This should much faster for all of us. I assume this was a desired feature in gaia, and I guess you have strong ideas on how it should work as you maintain gaia, which is fine. Who better to ensure it is all to your code standards than the core developers.

(You actually should have the power to make edits directly on this branch as well already)

Ok, let me take a stab at non-violent-communication here.

I understand you are feeling frustrated. It also seems to me you are feeling a bit impatient with the overall process. It seems you believe this PR should be merged quickly and only with basic and surface-level feedback from the SDK team. I also understand your need for this module to be merged or reach a completed state in order for you and your team to fulfill your contractual obligation to the ICF.

I request that you, in return, understand our limitations and due process. We, the SDK¹ team, make no stipulations or contractual agreements on the timelines for any piece of work to be merged by any contributor. So I am not sure why exactly you are frustrated by timelines here. In addition, there was never any mention from either the ICF or regen on due dates or timelines. We strive to maintain the best practices in the SDK. This means core (e.g. state-machine/module) PRs may take multiple rounds of review and feedback to reach their best possible state. Why? Because we want to provide a best-of-class SDK for blockchain developers and users.

If you are not happy with this process and/or are too impatient, I recommend you stop contributing to the SDK altogether. It is clear that you do not take too kindly or are too receptive to constructive criticism and are too impatient to see this through. I personally see nothing wrong with a PR taking 2 days vs 2-4 weeks to review and merge if it is a critical aspect of the state-machine upon which a multi-million dollar network is running atop of.

Currently just me because the two other members are focused on IBC.

alexanderbez · 2019-11-07T19:48:29Z

I will be happy to continue and provide the best feedback I can here. Also, note I believe this PR is nearly 98% complete.

jaekwon · 2019-11-07T19:51:50Z

x/feegrant/doc.go

+provides ways for specifying fee allowances such that delegating fees
+to another account can be done with clear and safe restrictions.
+
+A user would delegate fees to a user using MsgDelegateFeeAllowance and revoke


Seems like "delegate" is used inconsistently here. It should be a unidirection relationship but it appears to be used bidirecitonally?

A tx signer, say Alice, might "delegate" the responsibility of paying fees to someone else, say Bob.
But I wouldn't say that Bob in this case is delegating anything to Alice.

So I'd call this MsgAddFeeAllowance. or MsgRegisteerFeeAllowance.

I've been advocating for the language of grant and revoke, so how about MsgGrantFeeAllowance?

Oh, that is the name of the actual struct. Looks like the docs just need to be updated...

ethanfrey · 2019-11-08T10:48:10Z

Hi @bez, and thank you for your answer.

We do agree on two things. That a thorough review is very important for contributions to a state-machine running a multi-million dollar network. And that I should stop contributing to the sdk.

I do apologize for my impatience, which is caused by having a series of PRs building on this that are due within 4 weeks, and that I cannot meaningfully work on until this is merged. I would also like to point out that this long PR review process has had little effect on the quality of the resulting binary. Your comments are almost entirely based on documentation corrections, renaming, and some stylistic changes to make it look more like the other sdk code. I just reviewed all comments and the only ones that actually impact the code correctness of functionality were Fede asking me to add events to the Result, you requesting a change of queries to use callbacks instead of returning slices, and you and Aditya asking me to make a custom TX type instead of modifying StdTx (a decision that significantly complicated the work and that you yourself later questioned ).

I was happy to make the above changes, but they get lost in the level of nitpicking. In fact these nitpicks distract so much from the core functionality, that it lowers the focus on actual logic bugs. If I felt these reviews showed a deep understanding of the codebase and were productively leading to an increase in quality/performance/functionality, I would be much more acceptive, even if they did take long to occur.

In the end, I have no personal need for these PRs to ever go into the cosmos-sdk and even for my work with Regen, it would be much easier just to implement these models in Regen's app, or maybe just fork the sdk, as Binance and Irisnet both did. Adding these features to the core sdk was requested by AiB and ICF (even if not from you personally), and I would have assumed a more supportive attitude from the developers.

Finally, I want to recommend two articles to read when you have time. They both related to improving the culture of code reviews and I think will be beneficial for all future contributors to the SDK.

While I am not saying this is a fully toxic culture, I see many of the points of the first article reflected here, and a few of the second one. The point here is not to lay blame, but make suggestions for improving the process in the future. I think it would make many people more happy, and may help to increase the size of the core sdk team.

I will stop work on all open PRs and you and @aaronc can decide if you still want to try to merge them (or just close them) and who should take ownership of them. Feel free to make any desired changes. I feel a burden lifted from my shoulders, and I hope you feel less stressed now as well.

alexanderbez · 2019-11-08T14:24:27Z

I do apologize for my impatience, which is caused by having a series of PRs building on this that are due within 4 weeks.

From what I understand, this is a self-imposed deadline. No such deadline was made clear to the SDK team during contract negotiation. For if it had been, we would've clearly given direct pushback on it based on the foundation of resource constraints.

I would also like to point out that this long PR review process has had little effect on the quality of the resulting binary. Your comments are almost entirely based on documentation corrections, renaming, and some stylistic changes ...

I respectfully disagree. The last round of comments I made included a bug fix, a few design decisions that we are trying to make standard use of in the SDK (e.g. prefix stores which I even mentioned in a reference issue), and the complete lack of a spec (again, see evidence module PR for reference)! The last review, which seemed to have invoked this conversation, included changes which I believed improve the codebase (non-stylistically).

In fact these nitpicks distract so much from the core functionality, that it lowers the focus on actual logic bugs.

Again, I respectfully disagree. These are not nitpicking. These are changes to the fundamental design and implementation.

In the end, I have no personal need for these PRs to ever go into the cosmos-sdk and even for my work with Regen, it would be much easier just to implement these models in Regen's app, or maybe just fork the sdk, as Binance and Irisnet both did.

This is exactly what we want!!! Idk why people are using the term "fork" here though? We want developers and projects to develop their modules and publish them in their repos (not the entire SDK though)! The only thing missing a registry to explore all of these modules (which Fission Labs intends on working on).

Adding these features to the core sdk was requested by AiB and ICF (even if not from you personally), and I would have assumed a more supportive attitude from the developers.

Requested by AiB? This is news to me. No one on the SDK requested these be merged in the SDK team. If the ICF directly requested this, then we need to refine the process (which is pretty broken atm IMHO).

A few other notes:

I realize my initial response was not fully in the spirit of NVC, I made some mistakes there. Apologies (thanks for pointing that out @jaekwon).
I've been reading Google's Engineering Practices documentation and I feel like our review process is really not far off what can be considered ideal. Although some improvements can be made and I take full responsibility for improving it. I'm sorry you disagree with this. All changes I requested, I believe are in line with "improves the overall code health of the system being worked on, even if the CL isn’t perfect.".

That being said, I don't know the best way to proceed. I see the following options:

You can continue this work (unlikely).
@aaronc can finish off the last round of review in merge into the SDK.
I can finish off this work and merge into the SDK (I'd advise against this).
We close this PR entirely and have Regen be sole owners and main arbiters of the module. Either in cosmos/modules or in your own repo with the SDK providing advisory/audit feedback. (my personal preference for this PR).

fedekunze · 2019-12-02T19:01:33Z

You can continue this work (unlikely).

@aaronc can finish off the last round of review in merge into the SDK.

I can finish off this work and merge into the SDK (I'd advise against this).

We close this PR entirely and have Regen be sole owners and main arbiters of the module. Either in cosmos/modules or in your own repo with the SDK providing advisory/audit feedback. (my personal preference for this PR).

@aaronc or @ethanfrey please decide which alternative you will take with regards to this PR

aaronc · 2019-12-02T19:26:39Z

You can continue this work (unlikely).

@aaronc can finish off the last round of review in merge into the SDK.

I can finish off this work and merge into the SDK (I'd advise against this).

We close this PR entirely and have Regen be sole owners and main arbiters of the module. Either in cosmos/modules or in your own repo with the SDK providing advisory/audit feedback. (my personal preference for this PR).

@aaronc or @ethanfrey please decide which alternative you will take with regards to this PR

We have a meeting(s) pending with AIB and ICF to discuss next steps. Maybe you're not on that thread. Bez was on vacation and then I was out last week so we're still trying to organize the meeting.

alexanderbez · 2020-01-09T20:27:40Z

I will be taking over and pushing this over the finish line. However, I don't want to block the release of v0.38 on this, rather slate it for 0.39. Do you have any objections to this @aaronc?

aaronc · 2020-01-09T20:31:54Z

Thanks @alexanderbez. That's fine with me.

ethanfrey added 12 commits October 16, 2019 10:22

Add docs

abce938

Add BasicFeeAllowance implementation

ea6fe7d

Add expiration structs and complete basic fee

c46153c

Add delegation messages, add validation logic

42c17f5

Add keeper and helper structs

0c137cb

Add alias and handler to top level

67cb830

Add delegation module

f6ba9bc

Add basic querier

a002366

Add types tests

15c6361

Add types tests

410d1f1

More internal test coverage

f65db09

Solid internal test coverage

e0f6327

fedekunze requested a review from sunnya97 October 16, 2019 14:38

Expose Querier to top level module

93c9b7e

golangcibot reviewed Oct 16, 2019

View reviewed changes

ethanfrey added 6 commits October 16, 2019 18:59

Add FeeAccount to auth/types, like StdTx, SignDoc

7968441

Fix all tests in x/auth

7e82fb8

All tests pass

64d5159

Appease the Golang Linter

8b2b64b

Merge remote-tracking branch 'origin/master' into cg-key-management/f…

4e22edd

…ee-delegation-new

Add fee-account command line flag

be12b44

jackzampolin reviewed Oct 16, 2019

View reviewed changes

ethanfrey added 7 commits October 17, 2019 11:46

Start on DelegatedDeductFeeDecorator

c703e59

Cleanup the Decorator

907776f

Wire up delegation module in simapp

005afd9

add basic test for decorator (no delegation)

45c64d2

Table tests for deduct fees

87fd0ce

Table tests over all conditions of delegated fee decorator

d03c650

Build full ante handler stack and test it

e476c03

Update alias file, include ante

375f114

golangcibot reviewed Nov 7, 2019

View reviewed changes

x/feegrant/alias.go Show resolved Hide resolved

I do need nolint in alias.go

9af9f43

fedekunze approved these changes Nov 7, 2019

View reviewed changes

alexanderbez suggested changes Nov 7, 2019

View reviewed changes

ethanfrey commented Nov 7, 2019

View reviewed changes

Properly emit events in the handler. Use cosmos-sdk in amino types

3c390e5

jaekwon reviewed Nov 7, 2019

View reviewed changes

RiccardoM mentioned this pull request Nov 13, 2019

v0.38 Milestone #5172

Closed

4 tasks

fedekunze added the will-close label Dec 2, 2019

alexanderbez added this to the SDK Roadmap milestone Dec 11, 2019

aaronc mentioned this pull request Jan 15, 2020

Any progress in Subkeys Spec #5516

Closed

alexanderbez closed this Mar 9, 2020

alexanderbez mentioned this pull request Mar 9, 2020

Enable Fee Delegation #5768

Closed

15 tasks

alexanderbez removed this from the SDK Roadmap milestone Apr 2, 2020

aaronc mentioned this pull request Aug 17, 2020

Key Management Meta-issue #7074

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Fee Delegation #5207

Enable Fee Delegation #5207

ethanfrey commented Oct 16, 2019 •

edited

Loading

codecov bot commented Oct 16, 2019 •

edited

Loading

jackzampolin Oct 16, 2019

ethanfrey Oct 17, 2019 •

edited

Loading

ethanfrey Oct 17, 2019

fedekunze left a comment

fedekunze Nov 7, 2019

alexanderbez left a comment

alexanderbez Nov 7, 2019

ethanfrey Nov 7, 2019

alexanderbez Nov 7, 2019

ethanfrey Nov 7, 2019

alexanderbez Nov 7, 2019

ethanfrey Nov 7, 2019

alexanderbez Nov 7, 2019

ethanfrey Nov 7, 2019 •

edited

Loading

aaronc Nov 7, 2019 •

edited

Loading

aaronc Nov 7, 2019

alexanderbez Nov 7, 2019

ethanfrey Nov 7, 2019

ethanfrey left a comment

ethanfrey commented Nov 7, 2019 •

edited

Loading

alexanderbez commented Nov 7, 2019 •

edited

Loading

alexanderbez commented Nov 7, 2019

jaekwon Nov 7, 2019 •

edited

Loading

aaronc Nov 7, 2019

aaronc Nov 7, 2019

ethanfrey commented Nov 8, 2019 •

edited

Loading

alexanderbez commented Nov 8, 2019 •

edited

Loading

fedekunze commented Dec 2, 2019 •

edited

Loading

aaronc commented Dec 2, 2019

alexanderbez commented Jan 9, 2020

aaronc commented Jan 9, 2020

	func (suite *KeeperTestSuite) SetupTest() {
	checkTx := false
	app := simapp.Setup(checkTx)

	// get the app's codec and register custom testing types
	cdc := app.Codec()
	cdc.RegisterConcrete(types.TestEquivocationEvidence{}, "test/TestEquivocationEvidence", nil)

	// recreate keeper in order to use custom testing types
	evidenceKeeper := evidence.NewKeeper(
	cdc, app.GetKey(evidence.StoreKey), app.GetSubspace(evidence.ModuleName),
	evidence.DefaultCodespace,
	)
	router := evidence.NewRouter()
	router = router.AddRoute(types.TestEvidenceRouteEquivocation, types.TestEquivocationHandler(*evidenceKeeper))
	evidenceKeeper.SetRouter(router)

	suite.ctx = app.BaseApp.NewContext(checkTx, abci.Header{Height: 1})
	suite.querier = keeper.NewQuerier(*evidenceKeeper)
	suite.keeper = *evidenceKeeper
	}

Enable Fee Delegation #5207

Enable Fee Delegation #5207

Conversation

ethanfrey commented Oct 16, 2019 • edited Loading

codecov bot commented Oct 16, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ethanfrey Oct 17, 2019 • edited Loading

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

alexanderbez 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

ethanfrey Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

aaronc Nov 7, 2019 • 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

ethanfrey left a comment

Choose a reason for hiding this comment

ethanfrey commented Nov 7, 2019 • edited Loading

alexanderbez commented Nov 7, 2019 • edited Loading

alexanderbez commented Nov 7, 2019

jaekwon Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Nov 8, 2019 • edited Loading

alexanderbez commented Nov 8, 2019 • edited Loading

fedekunze commented Dec 2, 2019 • edited Loading

aaronc commented Dec 2, 2019

alexanderbez commented Jan 9, 2020

aaronc commented Jan 9, 2020

ethanfrey commented Oct 16, 2019 •

edited

Loading

codecov bot commented Oct 16, 2019 •

edited

Loading

ethanfrey Oct 17, 2019 •

edited

Loading

ethanfrey Nov 7, 2019 •

edited

Loading

aaronc Nov 7, 2019 •

edited

Loading

ethanfrey commented Nov 7, 2019 •

edited

Loading

alexanderbez commented Nov 7, 2019 •

edited

Loading

jaekwon Nov 7, 2019 •

edited

Loading

ethanfrey commented Nov 8, 2019 •

edited

Loading

alexanderbez commented Nov 8, 2019 •

edited

Loading

fedekunze commented Dec 2, 2019 •

edited

Loading