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

Add GET redelegation endpoint and querier #2182

Closed
4 tasks
fedekunze opened this issue Aug 29, 2018 · 19 comments · Fixed by #2559
Closed
4 tasks

Add GET redelegation endpoint and querier #2182

fedekunze opened this issue Aug 29, 2018 · 19 comments · Fixed by #2559
Assignees

Comments

@fedekunze
Copy link
Collaborator

fedekunze commented Aug 29, 2018

#2139 is missing redelegation querier and redelegation endpoint that needs to be implemented for #2113:
GET /stake/delegators/{delegatorAddr}/redelegations/validator_from/{validatorSrcAddr}/validator_to/{validatorDstAddr}.

Please note that the GetRedelegation keeper is already done


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ValarDragon
Copy link
Contributor

Seeing as the tag here was changed from if-have-time-prelaunch to prelaunch, I'm presuming its needed / blocking something for voyager?

@fedekunze
Copy link
Collaborator Author

Not really for Voyager, but mostly because of the LCD spec (#2113), which is labeled prelaunch

@fedekunze
Copy link
Collaborator Author

@faboweb @jackzampolin @cwgoes @HaoyangLiu do you think we require to have this functionality of querying a single redelegation ? According to Fabo it might not be required since clients will most of the time do query all of them at once. Thoughts ?

@jackzampolin
Copy link
Member

I think we need this functionality for individual delegation viewing screens. Also does the group of delegations endpoint allow for filtering by user? I have a feeling theres a lot of UI that would require the ability to show all the delegations by an individual user.

@sunnya97 sunnya97 self-assigned this Oct 22, 2018
@sunnya97
Copy link
Member

sunnya97 commented Oct 23, 2018

/stake/delegators/{delegatorAddr}/redelegations/validator_from/{validatorSrcAddr}/validator_to/{validatorDstAddr} seems like bad REST-ful API 😞

Can't we use the GET data field instead of URL params?

@alexanderbez
Copy link
Contributor

Seems like /stake/delegators/{delegatorAddr}/redelegations should be the resource GET endpoint with query params.

@jackzampolin
Copy link
Member

jackzampolin commented Oct 23, 2018

Agree with @alexanderbez and @sunnya97 here. We should push this into query params.

@fedekunze
Copy link
Collaborator Author

actually shouldn't /stake/delegators/{delegatorAddr}/redelegations/validator_from/{validatorSrcAddr}/validator_to/{validatorDstAddr} return an array of redelegations instead of a single one ? what if the user redelegates first only a partial amount of atoms and then the rest ? will be there a conflict with the key ? cc: @alexanderbez @cwgoes @jackzampolin @sunnya97 ?

@alexanderbez
Copy link
Contributor

alexanderbez commented Oct 23, 2018

Huh? What wrong with /stake/delegators/{delegatorAddr}/redelegations?validator_from={validatorSrcAddr}&validator_to={validatorDstAddr}?

Obviously we'd perform sanity/validation checks.

@fedekunze
Copy link
Collaborator Author

fedekunze commented Oct 23, 2018

Huh? What wrong with /stake/delegators/{delegatorAddr}/redelegations?validator_from={validatorSrcAddr}&validator_to={validatorDstAddr}?

I'm not opposed to use query parameters

@alexanderbez
Copy link
Contributor

Great! And /stake/delegators/{delegatorAddr}/redelegations would get you all the redelegations I presume.

@sunnya97
Copy link
Member

sunnya97 commented Oct 23, 2018

It should be /stake/delegators/{delegatorAddr}/redelegation?validator_from={validatorSrcAddr}&validator_to={validatorDstAddr} right? Singular redelegation, not plural.

Alternatively, I can do create the endpoint:
/stake/redelegations?delegator={delegatorAddr}&validator_from={validatorSrcAddr}&validator_to={validatorDstAddr}
and use that for all redelegations queries, including all those from a specific validator (for example, even replacing the /stake/validators/{validatorAddr}/redelegations). I actually prefer this model.

@alexanderbez
Copy link
Contributor

Ahhh yes there can only be a single redelegation, so yes singular. I like the later model you mentioned as well.

@fedekunze
Copy link
Collaborator Author

I'd go with the first one with queries bc /stake/redelegations (without any query) doesn't make sense (why would you want to return all redelegations from everyone). So I propose:

  • /stake/delegators/{delegatorAddr}/redelegation?validator_from={validatorSrcAddr}&validator_to={validatorDstAddr}: return all reds from a delegator (+ query)
  • /stake/validators/{validatorAddr}/redelegation?delegator={delegatorAddr}&validator={validatorAddr}: return all reds from a validator (from or to + query)

@sunnya97
Copy link
Member

sunnya97 commented Oct 23, 2018

why would you want to return all redelegations from everyone

I can see a block explorer or some cool delegation visualizer wanting to do that

/stake/validators/{validatorAddr}/redelegation?delegator={delegatorAddr}&validator={validatorAddr}: return all reds from a validator (from or to + query)

In this one, is this redelegations to or from a specific validator?

If you really wanted to do this, this should be split into two endpoints:

  • /stake/validators/{validatorAddr}/outgoing_redelegations?validator_to={dstValidatorAddr}&delegator={delegatorAddr}
  • /stake/validators/{validatorAddr}/incoming_redelegations?validator_from={srcValidatorAddr}&delegator={delegatorAddr}

But I don't see why/how is this cleaner than having a single endpoint?

@faboweb
Copy link
Contributor

faboweb commented Nov 29, 2018

I can see a block explorer or some cool delegation visualizer wanting to do that

you can still query all transactions of a certain type by using /txs, why misuse this endpoint for that purpose?

Ahhh yes there can only be a single redelegation, so yes singular.

no, resources are always in plural. for one delegator/validator there can be multiple redelegations that are filtered by the query parameters

Delegator redelegations endpoint I could go with:

/stake/delegators/{delegatorAddr}/redelegations?validator_from={validatorSrcAddr}&validator_to={validatorDstAddr}

For the validator redelegations I think the endpoint is already pretty solid:

/stake/validators/{validatorAddr}/redelegations?delegator={delegatorAddr}&validator={validatorAddr}: return all reds from a validator (from or to + query)

Clients can filter on incoming and outgoing redelegations based on the tx. Plus they only need to do one request instead of two.

Did any consumer of the API complain about its current form?

@sunnya97
Copy link
Member

Having to use txs to query all redelegations or to filter between incoming vs outgoing redelegations seems suboptimal. Using tx tags is good for listening for events, but if a query can be done using state, that should be preferred.

@alexanderbez
Copy link
Contributor

The same could be said for governance votes and deposits. Where do we draw the line?

@jackzampolin
Copy link
Member

@alexanderbez This is a REALLY good question and I think that performance and feedback from clients should really drive this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants