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

RESTful governance endpoints #1357

Merged
merged 12 commits into from
Jun 27, 2018
Merged

RESTful governance endpoints #1357

merged 12 commits into from
Jun 27, 2018

Conversation

sunnya97
Copy link
Member

@sunnya97 sunnya97 commented Jun 24, 2018

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated Gaia/Examples
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

closes #1284

@sunnya97 sunnya97 requested review from faboweb and cwgoes June 24, 2018 22:14
@sunnya97 sunnya97 requested a review from ebuchman as a code owner June 24, 2018 22:14
@sunnya97 sunnya97 removed the request for review from ebuchman June 24, 2018 22:15
@sunnya97
Copy link
Member Author

Something weird happening in the CI. Says authentication required to pull from the docker repo

@sunnya97
Copy link
Member Author

@rigelrozanski How do I get it to rerun the circle tests?

@ValarDragon
Copy link
Contributor

If you force push to your current branch, that may refresh it. Otherwhise, merge in develop, or rerun the tests manually. (Click on details for each test, then rerun via ssh in the top right)

bechVoterAddr := vars[RestVoter]

if len(strProposalID) == 0 {
w.WriteHeader(http.StatusBadRequest)
// w.WriteHeader(http.StatusBadRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need these headers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh oops, commented these out for some debugging thing. Will uncomment them

// REST

// Rest Deposits
type DepositRest struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's come up with a naming schema for REST output

Copy link
Contributor

Choose a reason for hiding this comment

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

pulling in @cwgoes @rigelrozanski

Copy link
Contributor

Choose a reason for hiding this comment

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

DepositRest is fine IMO

@faboweb
Copy link
Contributor

faboweb commented Jun 25, 2018

Looks pretty nice :)

@sunnya97
Copy link
Member Author

sunnya97 commented Jun 27, 2018

@ValarDragon Did you figure out the CI problem? listen tcp 0.0.0.0:36658: bind: address already in use

@codecov
Copy link

codecov bot commented Jun 27, 2018

Codecov Report

Merging #1357 into develop will decrease coverage by 0.06%.
The diff coverage is 40%.

@@             Coverage Diff             @@
##           develop    #1357      +/-   ##
===========================================
- Coverage    63.78%   63.72%   -0.07%     
===========================================
  Files          109      109              
  Lines         6028     6034       +6     
===========================================
  Hits          3845     3845              
- Misses        1964     1970       +6     
  Partials       219      219


// Turn any Deposit to a DepositRest
func DepositToRest(deposit Deposit) DepositRest {
bechAddr, _ := sdk.Bech32ifyAcc(deposit.Depositer)
Copy link
Contributor

Choose a reason for hiding this comment

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

sdk.MustBech32ifyAcc

@@ -264,7 +264,7 @@ func (keeper Keeper) AddDeposit(ctx sdk.Context, proposalID int64, depositerAddr
// Add or update deposit object
currDeposit, found := keeper.GetDeposit(ctx, proposalID, depositerAddr)
if !found {
newDeposit := Deposit{depositerAddr, depositAmount}
newDeposit := Deposit{depositerAddr, proposalID, depositAmount}
Copy link
Contributor

@cwgoes cwgoes Jun 27, 2018

Choose a reason for hiding this comment

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

Why do we need to store a copy of the proposal ID in each deposit? Can't we retrieve it from the key?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Technically we don't even need to to store the depositerAddr either.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - #1419.

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.

Tested ACK

@cwgoes cwgoes merged commit 4effa6f into develop Jun 27, 2018
@cwgoes cwgoes deleted the sunny/governance_rest branch June 27, 2018 23:50
adrianbrink pushed a commit that referenced this pull request Jul 2, 2018
* get deposit rest endpoint
* query proposals
* changelog
* fixed commented out headers
* fixed undeterministic tests
* increase circle test timeout
* MustBech32ifyAcc
* asdf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change governance REST endpoints to a more REST-ful structure
5 participants