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

Return tags through hooks and in x/stake #3122

Closed
cwgoes opened this issue Dec 14, 2018 · 11 comments
Closed

Return tags through hooks and in x/stake #3122

cwgoes opened this issue Dec 14, 2018 · 11 comments
Assignees
Labels
C:x/distribution distribution module related C:x/staking S:blocked Status: Blocked T: UX

Comments

@cwgoes
Copy link
Contributor

cwgoes commented Dec 14, 2018

Unsolved problem, at the moment apparently we just don't, e.g. https://github.com/cosmos/cosmos-sdk/blob/develop/x/stake/keeper/delegation.go#L536

cc @rigelrozanski thoughts?

I wonder if we should instead find a way to pass "events" (lists of tags) through the context in a more natural fashion - they really are intended to be logs, not variables to manipulate.

@rigelrozanski
Copy link
Contributor

Let me get this straight, so when we certain transactions are called, hooks are also called. Normally we would only return tags for the given transaction, however it would also be useful to have tags for the events which occur on hooks being called. - the current system only allows for a single set of tags to be returned per message - but because of the hooks being called we should really be returning a list of all the events as a group of tags per message.

Is that roughly what you're talking about?

if so, then yes I agree this is useful information which we should be returning so we can sort navigate through all those events adequately 👍

Shouldn't be too too difficult but sounds like it requires some fundamental restructuring of the sdk tagging system

@cwgoes
Copy link
Contributor Author

cwgoes commented Dec 15, 2018

Is that roughly what you're talking about?

Yes.

Shouldn't be too too difficult but sounds like it requires some fundamental restructuring of the sdk tagging system.

Yes, I think it does. I think it should be structured much more like logs, e.g. a ctx.Event() call.

@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 21, 2019

cc @sunnya97 - interested / thoughts?

@rigelrozanski
Copy link
Contributor

@cwgoes thoughts on the label change here?

@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 24, 2019

Hmm, this will be pretty confusing for users (cc @jbibla) I think.

@jbibla
Copy link
Contributor

jbibla commented Jan 25, 2019

hmmm. the first link 404s so i'm not sure what you're referring to exactly or how it might impact the user (or voyager).

@fedekunze might have a better idea of what's being discussed here.

@sunnya97 sunnya97 added this to the v0.31.0 (Launch RC) milestone Jan 29, 2019
@jackzampolin jackzampolin added the S:blocked Status: Blocked label Jan 31, 2019
@cwgoes
Copy link
Contributor Author

cwgoes commented Jan 31, 2019

(blocked on determining whether Tendermint will have the requisite tag indexing to make this useful)

@alexanderbez
Copy link
Contributor

Ideally, I'd like to close this issue and create a new issue that updates the tagging system to an event-based system. Essentially, any part of a module that has access to a ctx, can log/trigger a tag/event. These are all then collected at the end of the tx. This will allow us to really tag any kind of event or action we want (eg. automatic withdrawal of rewards -- which the community really wants).

This is still blocked on the next breaking release of TM.

@alexanderbez alexanderbez mentioned this issue May 21, 2019
4 tasks
@cwgoes
Copy link
Contributor Author

cwgoes commented Jul 4, 2019

@alexanderbez Has this been done (I know the events have - but have the additional tags?)

@alexanderbez
Copy link
Contributor

@cwgoes we now have the ability to emit events from anywhere where a ctx is provided.

Take a look at the staking events here and see if you think anything is missing. Otherwise, yes, I believe we can close this issue.

@cwgoes
Copy link
Contributor Author

cwgoes commented Jul 5, 2019

@cwgoes we now have the ability to emit events from anywhere where a ctx is provided.

Take a look at the staking events here and see if you think anything is missing. Otherwise, yes, I believe we can close this issue.

I think there was specific interest in details of fee distribution events (e.g. on redelegating) since some delegators / validators needed dates & amounts for tax purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/distribution distribution module related C:x/staking S:blocked Status: Blocked T: UX
Projects
None yet
Development

No branches or pull requests

7 participants