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

Custom MsgMultiSend Handler (x/bank fork) #3807

Merged
merged 18 commits into from
Mar 6, 2019
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Mar 5, 2019

  • Temporarily fork x/bank router into the gaia app modules to allow a custom message handling.
  • Remove send_enabled genesis param

closes: #3802

credit: @sunnya97

/cc @jackzampolin @jaekwon @rigelrozanski @cwgoes


  • 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 entries in PENDING.md with issue #

  • 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)

@alexanderbez alexanderbez added WIP T: State Machine Breaking State machine breaking changes (impacts consensus). labels Mar 5, 2019
@alexanderbez alexanderbez requested review from cwgoes, rigelrozanski, jaekwon, zmanian and sunnya97 and removed request for cwgoes March 5, 2019 16:45
@alexanderbez
Copy link
Contributor Author

If this is agreed upon, I'll go ahead and wrap up this PR.

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #3807 into develop will increase coverage by <.01%.
The diff coverage is 63.88%.

@@             Coverage Diff             @@
##           develop    #3807      +/-   ##
===========================================
+ Coverage    60.88%   60.88%   +<.01%     
===========================================
  Files          191      192       +1     
  Lines        14180    14215      +35     
===========================================
+ Hits          8633     8655      +22     
- Misses        4991     4999       +8     
- Partials       556      561       +5

@alexanderbez alexanderbez marked this pull request as ready for review March 5, 2019 21:53
@@ -0,0 +1,210 @@
package bank
Copy link
Member

Choose a reason for hiding this comment

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

love these testcases @alexanderbez !

PENDING.md Outdated Show resolved Hide resolved
PENDING.md Outdated Show resolved Hide resolved
cmd/gaia/app/x/bank/handler.go Outdated Show resolved Hide resolved
cmd/gaia/app/x/bank/handler.go Outdated Show resolved Hide resolved
@sunnya97
Copy link
Member

sunnya97 commented Mar 6, 2019

Why are we using both the genesis param and codepath changes? Then when we want to activate transfers, we have to change both? We could be just using the codepath, and remove SendsEnabled as a genesis param completely. It's not a param we plan to use other than for this one hard fork anyways (it will be depreciated soon by the circuit breaker).

@cwgoes
Copy link
Contributor

cwgoes commented Mar 6, 2019

Why are we using both the genesis param and codepath changes? Then when we want to activate transfers, we have to change both? We could be just using the codepath, and remove SendsEnabled as a genesis param completely. It's not a param we plan to use other than for this one hard fork anyways (it will be depreciated soon by the circuit breaker).

The mechanism for the circuit breaker might be just to use that same parameter - but I agree for now, let's remove the parameter for enabling / disabling sends in this PR to avoid confusion.

PENDING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Agreed with @sunnya97 on removing the parameter, otherwise structure looks fine.

cmd/gaia/app/x/bank/handler.go Show resolved Hide resolved
cmd/gaia/app/x/bank/handler.go Show resolved Hide resolved
cmd/gaia/app/x/bank/handler.go Show resolved Hide resolved
cwgoes and others added 5 commits March 6, 2019 08:50
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
Co-Authored-By: alexanderbez <alexanderbez@users.noreply.github.com>
@alexanderbez
Copy link
Contributor Author

@sunnya97 @cwgoes we can remove the genesis param, but all the LCD and integration tests relying on sends will fail...because they're disabled. So we must keep the parameter or create an app somehow with the original router.

@cwgoes
Copy link
Contributor

cwgoes commented Mar 6, 2019

I see, OK - as-is is fine with me then.

@cwgoes cwgoes merged commit bf7cbbb into develop Mar 6, 2019
@cwgoes cwgoes deleted the bez/3802-multimsg-alt branch March 6, 2019 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/bank T: State Machine Breaking State machine breaking changes (impacts consensus).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support New Validators at Launch
4 participants