Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass in sdk.Context to router.Route() #5452

Merged
merged 19 commits into from
Jan 4, 2020
Merged

Pass in sdk.Context to router.Route() #5452

merged 19 commits into from
Jan 4, 2020

Conversation

sunnya97
Copy link
Member

@sunnya97 sunnya97 commented Dec 27, 2019

Adding this will allow apps to create more customizable msg Routers. For example, this one I wrote here that allows governance to toggle on and off certain routes.

For a router, that doesn't want to make use of the ctx (like the default one in baseapp), it can just ignore it.

closes: #5455


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)

@sunnya97 sunnya97 added the R4R label Dec 27, 2019
@sunnya97 sunnya97 changed the title Add sdk.Context to router.Route() Pass in sdk.Context to router.Route() Dec 27, 2019
@alexanderbez
Copy link
Contributor

Mhhhh, something about routers being stateful doesn't sit right with me. Is the point to disable certain message routes? Typically, this is done via parameters but that's not the best UX, I agree. There is also this issue on Hierarchical Msg Routing, which I think has some overlap here.

Would like to get other opinions here @AdityaSripal @fedekunze

@sunnya97
Copy link
Member Author

sunnya97 commented Dec 29, 2019

@alexanderbez Is the point to disable certain message routes?

Well, that's one example, which I wrote a module for. There are probably other reasons to do it as well such as upgrading a specific module (assuming modules are hot-loaded using CosmWasm or something).

Typically, this is done via parameters but that's not the best UX, I agree.

Also, genesis parameters can't be changed. I want governance to be able to update on an active chain. I do use the paramstore in the togglerouter module, but the router needs the ctx in order to get the current state of the params. We'll also need something like this for the circuit breaker.

@alexanderbez alexanderbez mentioned this pull request Jan 1, 2020
4 tasks
@alexanderbez alexanderbez added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Jan 1, 2020
@alexanderbez
Copy link
Contributor

ref: #5455 (comment)

I don't have any strong objections here. Concept ACK unless others can state strong objective reasons for not going forward.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit, otherwise ACK.

CHANGELOG.md Outdated Show resolved Hide resolved
baseapp/router.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

CHANGELOG.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 4, 2020

Codecov Report

Merging #5452 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5452      +/-   ##
==========================================
+ Coverage    54.4%   54.42%   +0.02%     
==========================================
  Files         313      313              
  Lines       18850    18854       +4     
==========================================
+ Hits        10255    10261       +6     
+ Misses       7808     7806       -2     
  Partials      787      787
Impacted Files Coverage Δ
baseapp/router.go 100% <100%> (ø) ⬆️
baseapp/baseapp.go 71.37% <100%> (ø) ⬆️
baseapp/options.go 73.13% <100%> (+1.7%) ⬆️
x/mock/app.go 64.18% <0%> (+1.35%) ⬆️

@alexanderbez alexanderbez merged commit 17d639a into master Jan 4, 2020
@alexanderbez alexanderbez deleted the ctx_msg_router branch January 4, 2020 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:baseapp T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sdk.Router should be stateful
4 participants