R4R: Redelegation Querier #2559
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -586,19 +586,23 @@ func TestBonding(t *testing.T) { | |
| require.Len(t, delegatorDels, 1) | ||
| require.Equal(t, "30.0000000000", delegatorDels[0].GetShares().String()) | ||
|
|
||
| redelegation := getRedelegations(t, port, addr, operAddrs[0], operAddrs[1]) | ||
| require.Len(t, redelegation, 1) | ||
| require.Equal(t, "30", redelegation[0].Balance.Amount.String()) | ||
|
|
||
| delegatorUbds := getDelegatorUnbondingDelegations(t, port, addr) | ||
| require.Len(t, delegatorUbds, 1) | ||
| require.Equal(t, "30", delegatorUbds[0].Balance.Amount.String()) | ||
|
|
||
| delegatorReds := getRedelegations(t, port, addr, nil, nil) | ||
| require.Len(t, delegatorReds, 1) | ||
| require.Equal(t, "30", delegatorReds[0].Balance.Amount.String()) | ||
|
|
||
| validatorUbds := getValidatorUnbondingDelegations(t, port, operAddrs[0]) | ||
| require.Len(t, validatorUbds, 1) | ||
| require.Equal(t, "30", validatorUbds[0].Balance.Amount.String()) | ||
|
|
||
| validatorReds := getRedelegations(t, port, nil, operAddrs[0], nil) | ||
| require.Len(t, validatorReds, 1) | ||
| require.Equal(t, "30", validatorReds[0].Balance.Amount.String()) | ||
|
|
||
|
|
@@ -1043,16 +1047,31 @@ func getDelegatorUnbondingDelegations(t *testing.T, port string, delegatorAddr s | |
| return ubds | ||
| } | ||
|
|
||
| func getRedelegations(t *testing.T, port string, delegatorAddr sdk.AccAddress, srcValidatorAddr sdk.ValAddress, dstValidatorAddr sdk.ValAddress) []stake.Redelegation { | ||
| var res *http.Response | ||
| var body string | ||
|
|
||
| endpoint := "/stake/redelegations?" | ||
|
There was a problem hiding this comment. I still think this is a terrible design, we should only add the query parameters to There was a problem hiding this comment. The thing I don't like about There was a problem hiding this comment. I would agree with @fedekunze here. I don't think how we have the data structured internally matters to users. Presenting the data in a way that is logically consistent (delegators have redelegations) is a great idea. There was a problem hiding this comment. What are /stake/delegators/<>/redelegations vs /stake/validators/<>/redelegations? I think over-emphasis on the URL design of API endpoints often leads to premature design constraints that end up causing more headache down the line. It is simpler to model the API after what is modeled in the implementation. REST isn't just about pretty URLs... there's nothing wrong with query parameters, esp given a context where we will want to provide filters etc. I favor Sunny's approach here, it's faster to execute and less brittle going into the future. The end users here are developers who can deal with query parameters. We're not talking about website URLs, which is a different story. There was a problem hiding this comment.
I think simple to implement is not what we want with an API. An API should be consistent for quiet some time so developers don't need to adjust to constant changes (this is frustrating from experience). We should come up with a design that reflects the expectation of the developers that want to use the API. Let's invite more of them to stakeholder meetings and discuss together with them the API design. |
||
|
|
||
| if !delegatorAddr.Empty() { | ||
| endpoint += fmt.Sprintf("delegator=%s&", delegatorAddr) | ||
|
There was a problem hiding this comment. consider using https://golang.org/pkg/strings/#Builder There was a problem hiding this comment. hmm interesting - yeah seems like it would make the code a bit smaller - but I don't see builders often so might just be more confusing for new devs... but then again maybe we should all learn! There was a problem hiding this comment. I think in this situation, it is simple enough that we can just use the |
||
| } | ||
| if !srcValidatorAddr.Empty() { | ||
| endpoint += fmt.Sprintf("validator_from=%s&", srcValidatorAddr) | ||
| } | ||
| if !dstValidatorAddr.Empty() { | ||
| endpoint += fmt.Sprintf("validator_to=%s&", dstValidatorAddr) | ||
| } | ||
|
|
||
| res, body = Request(t, port, "GET", endpoint, nil) | ||
|
|
||
| require.Equal(t, http.StatusOK, res.StatusCode, body) | ||
|
|
||
| var redels []stake.Redelegation | ||
| err := cdc.UnmarshalJSON([]byte(body), &redels) | ||
| require.Nil(t, err) | ||
|
|
||
| return redels | ||
| } | ||
|
|
||
| func getBondingTxs(t *testing.T, port string, delegatorAddr sdk.AccAddress, query string) []tx.Info { | ||
|
|
@@ -1254,17 +1273,6 @@ func getValidatorUnbondingDelegations(t *testing.T, port string, validatorAddr s | |
| return ubds | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. this should stay the same, see comment above |
||
| // ============= Governance Module ================ | ||
|
|
||
| func getDepositParam(t *testing.T, port string) gov.DepositParams { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have been in breaking changes cc: @sunnya97 @jackzampolin. I'll update it