Simplify context #4706

ethanfrey · 2019-07-10T16:39:45Z

I will polish this PR a bit more if it is deemed acceptable, but looking for first responses from devs.

I am getting back into sdk development after some time and find a number of places that seem needless complex - adding unneeded confusion. Going from the go maxim that simpler is better, I would like to offer a number of cleanup PRs. Not sure the best place.

Context is some odd object which is kind of like context.Context but then really like a simple immutible struct with getters and a setter than returns a modified copy of the object. Since the only place to make use of Value() and GetValue() outside of context.go was some governance test code, I figured to just streamline it. I left in an embedded context that can be used like in stdlib for that one case (which maybe is not needed?).

Besides that small change, all cleanup was limited to the types/context*.go files. I would like to rename Context to Request, which better represents the meaning of this object, and is in alignment with standard idioms used in go (web) frameworks. I just provide a type alias now to hint at future directions.

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.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: clog add [section] [stanza] [message]
rereviewed 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-07-10T16:45:31Z

Codecov Report

Merging #4706 into master will decrease coverage by 0.12%.
The diff coverage is 71.01%.

@@            Coverage Diff             @@
##           master    #4706      +/-   ##
==========================================
- Coverage   54.15%   54.03%   -0.13%     
==========================================
  Files         272      271       -1     
  Lines       17349    17307      -42     
==========================================
- Hits         9395     9351      -44     
- Misses       7271     7274       +3     
+ Partials      683      682       -1

jackzampolin · 2019-07-10T16:48:10Z

.pending/breaking/sdk/4706-Simplify-context

+
+Migration guide:
+`ctx = ctx.WithValue(contextKeyBadProposal, false)` ->
+`ctx = ctx.WithContext(context.WithValue(ctx.Context(), contextKeyBadProposal, false))`


Are there any instances of these calls in the code? Seems like based on the diff this is a minimal change.

in x/gov/test_common.go and x/gov/end_blocker_test.go to control failure mode or a test handler.

jackzampolin · 2019-07-10T16:49:23Z

types/context.go

+
+// clone the header before returning
+func (c Context) BlockHeader() abci.Header {
+	// TODO: figure out clone better


Guessing this todo will get worked out if you decide to go ahead with this approach.

yes, it paniced about trying to clone the type, not sure why...

I will ensure there are properly cloned if approved

types/context_test.go

x/gov/endblocker_test.go

jackzampolin · 2019-07-10T16:52:42Z

This gets a 👍 on direction from me. Seems to make the code much cleaner! Thanks for the contribution!

rigelrozanski

generally I agree that, because most of the context fields are being used there isn't much point in storing them with decorators

types/context.go

.pending/breaking/sdk/4706-Simplify-context

alessio

I like this PR's approach. The rationale is sound and all makes sense.
So far so good, I'm ready to ACK this once all comments are addressed

ethanfrey · 2019-07-10T19:56:17Z

I addressed all of the code comments here, except for the protobuf Clone.
I will discuss the design comment above in an issue.

alessio

Nihil obstat from me - @ethanfrey it would be appreciated if you could scan through the SDK docs and ensure all references are up to date.

ethanfrey · 2019-07-11T10:21:56Z

Thanks @alessio I will do a deeper read through docs and polish of the last edges here.

Glad to see you like the rename as well. I just added an optional type alias here:
type Request = Context
so people can refer to it as sdk.Request in their Handlers if they wish.

But if there is support, I would make a separate PR for a larger rename, updating all references to Context in sdk, but keep backwards compatibility with external libs with:
type Context = Request

gogo/protobuf#519 Seems like the old code never properly cloned this, which was not easily detectable through so many layers of indirection

ethanfrey · 2019-07-11T15:21:10Z

I fixed the protobuf clone issue, seems to be a time.Time problem.
And shows they were never actually Cloned in the older code

ethanfrey · 2019-07-11T15:26:02Z

Context appears many places in the docs (so renaming to Request is a big work).
I did minor godoc updates.

Otherwise, I think this is done

types/context.go

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

fedekunze

💯 💯

ethanfrey · 2019-07-16T12:01:50Z

Any progress on the review process? I think I addressed all concerns raised.

I also believe it is no longer API breaking, as I did add Value() and WithValue() back to sdk.Context. It doesn't inherit the other fields from Context (such as Done() and Error()), but I highly doubt anyone was using those, so I am unsure of the API breaking nature of this PR.

fedekunze · 2019-07-16T12:26:45Z

@ethanfrey, someone with merge rights will have to merge it cc: @alessio @jackzampolin @alexanderbez @rigelrozanski. I believe Bez will want to review this once he comes back from vacations tomorrow

ethanfrey · 2019-07-16T20:59:16Z

Thanks @alessio

alexanderbez · 2019-07-18T18:58:22Z

Took a look at the PR and the changes look reasonable to me, although I'm not sure I'm necessarily in favor of having to require the sdk.Context contain fields for the primary getters (e.g. txBytes). I suppose this is fine for now.

Also, what is the harm of using ctx.WithValue(...) over the new preferred API where I have to manually get the context? In the SDK, I cannot see where the API would be preferred.

ethanfrey · 2019-07-27T13:55:47Z

Hi @alexanderbez my point was that we only use this With Value in some governance test code, and in general, we should not encourage this usage of arbitrary data, when we really use it for structured data.

I left the ability to use it as such and no breaking api changes, but push towards a clear and simple struct which is easy to understand. In general, the simplest solution that satisfies the requirements is easier to understand, easier to change later and brings less technical debt.

This seemed like low hanging fruit to clean up, as I was not sure all the ways it was used in the Sdk without searching a lot

## Description This: * addresses an issue with the way `WrapSDKContext` was done in that it doesn't use the `context.Context` that is already inside `sdk.Context` * it allows `sdk.Context` to be used as a `context.Context` directly * it also walks back the deprecation of `Value` and `WithValue` from #4706 as I actually think these are useful, and trying to put everything directly into `Context` as a bag of variables rather than attaching it using something like `WithValue` makes the SDK less extensible --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

ethanfrey added 3 commits July 10, 2019 16:19

Simplify Context into a proper structure

47cf556

Fix build errors

1001022

All tests pass

464168d

ethanfrey requested review from alessio, alexanderbez, jackzampolin and rigelrozanski as code owners July 10, 2019 16:39

ethanfrey added 2 commits July 10, 2019 18:40

Cleanup

cd9ac34

Add changelog entry

2a665da

jackzampolin reviewed Jul 10, 2019

View reviewed changes

types/context_test.go Show resolved Hide resolved

jackzampolin reviewed Jul 10, 2019

View reviewed changes

x/gov/endblocker_test.go Outdated Show resolved Hide resolved

rigelrozanski reviewed Jul 10, 2019

View reviewed changes

types/context.go Show resolved Hide resolved

alessio reviewed Jul 10, 2019

View reviewed changes

.pending/breaking/sdk/4706-Simplify-context Outdated Show resolved Hide resolved

alessio reviewed Jul 10, 2019

View reviewed changes

Addressed PR comments

ae32c74

alessio approved these changes Jul 11, 2019

View reviewed changes

ethanfrey added 3 commits July 11, 2019 16:56

Clone protobuf header/consensus and add tests

1f631f5

Reproduced error with cloning non-UTC time

346e3a2

Fix Clone by forcing all Header.Time in Context to be UTC

a0f6e4d

gogo/protobuf#519 Seems like the old code never properly cloned this, which was not easily detectable through so many layers of indirection

Update types.Context docs

c38363d

fedekunze reviewed Jul 11, 2019

View reviewed changes

types/context.go Outdated Show resolved Hide resolved

types/context.go Outdated Show resolved Hide resolved

types/context.go Outdated Show resolved Hide resolved

Apply suggestions from code review

93d98c4

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

ethanfrey force-pushed the simplify-context branch from 48c7a9c to 93d98c4 Compare July 12, 2019 10:13

fedekunze approved these changes Jul 15, 2019

View reviewed changes

fedekunze added T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). R4R labels Jul 15, 2019

fedekunze removed the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Jul 16, 2019

alessio merged commit d3bb9f5 into cosmos:master Jul 16, 2019

ethanfrey deleted the simplify-context branch July 16, 2019 20:58

aaronc mentioned this pull request Jan 12, 2022

feat: make sdk.Context implement context.Context #10941

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify context #4706

Simplify context #4706

ethanfrey commented Jul 10, 2019 •

edited

Loading

codecov bot commented Jul 10, 2019 •

edited

Loading

jackzampolin Jul 10, 2019

ethanfrey Jul 10, 2019

jackzampolin Jul 10, 2019

ethanfrey Jul 10, 2019

jackzampolin commented Jul 10, 2019

rigelrozanski left a comment

alessio left a comment

ethanfrey commented Jul 10, 2019

alessio left a comment

ethanfrey commented Jul 11, 2019

ethanfrey commented Jul 11, 2019

ethanfrey commented Jul 11, 2019

fedekunze left a comment

ethanfrey commented Jul 16, 2019

fedekunze commented Jul 16, 2019

ethanfrey commented Jul 16, 2019

alexanderbez commented Jul 18, 2019

ethanfrey commented Jul 27, 2019

Simplify context #4706

Simplify context #4706

Conversation

ethanfrey commented Jul 10, 2019 • edited Loading

codecov bot commented Jul 10, 2019 • edited Loading

Codecov Report

jackzampolin Jul 10, 2019

Choose a reason for hiding this comment

ethanfrey Jul 10, 2019

Choose a reason for hiding this comment

jackzampolin Jul 10, 2019

Choose a reason for hiding this comment

ethanfrey Jul 10, 2019

Choose a reason for hiding this comment

jackzampolin commented Jul 10, 2019

rigelrozanski left a comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

ethanfrey commented Jul 10, 2019

alessio left a comment

Choose a reason for hiding this comment

ethanfrey commented Jul 11, 2019

ethanfrey commented Jul 11, 2019

ethanfrey commented Jul 11, 2019

fedekunze left a comment

Choose a reason for hiding this comment

ethanfrey commented Jul 16, 2019

fedekunze commented Jul 16, 2019

ethanfrey commented Jul 16, 2019

alexanderbez commented Jul 18, 2019

ethanfrey commented Jul 27, 2019

ethanfrey commented Jul 10, 2019 •

edited

Loading

codecov bot commented Jul 10, 2019 •

edited

Loading