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

EmitTypedEvent Escapes Strings #12592

Closed
4 tasks
alexpGH opened this issue Jul 15, 2022 · 8 comments
Closed
4 tasks

EmitTypedEvent Escapes Strings #12592

alexpGH opened this issue Jul 15, 2022 · 8 comments

Comments

@alexpGH
Copy link

alexpGH commented Jul 15, 2022

Summary of Bug

C:x/authz module uses extra " around value fields in events for grant transactions, e.g. granter, grantee, msg_type_url, which results in query results as e.g.:

- attributes:
  - index: true
    key: msg_type_url
    value: '"/cosmos.staking.v1beta1.MsgDelegate"'
  - index: true
    key: granter
    value: '"cosmos19pyadrsglm6hamfk06wjnng6q2fvclqdqd97rz"'
  - index: true
    key: grantee
    value: '"cosmos12lnlzxnayzydspgj740f4u9wavnavkeg57eavw"'
  type: cosmos.authz.v1beta1.EventGrant

which however should be

- attributes:
  - index: true
    key: msg_type_url
    value: /cosmos.staking.v1beta1.MsgDelegate
  - index: true
    key: granter
    value: cosmos19pyadrsglm6hamfk06wjnng6q2fvclqdqd97rz
  - index: true
    key: grantee
    value: cosmos12lnlzxnayzydspgj740f4u9wavnavkeg57eavw
  type: cosmos.authz.v1beta1.EventGrant

I could not find a related bug report.

Version

current main branch head, commit 01b4a42

Steps to Reproduce

issue a grant tx
./simd tx authz grant $(./simd keys show b9lab -a) generic --msg-type /cosmos.staking.v1beta1.MsgDelegate --from student --chain-id test-chain
and simd query tx hash it


For Admin Use

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

alexanderbez commented Jul 17, 2022

So these events are emitted using the new typed-events approach, as you can see here:

	return ctx.EventManager().EmitTypedEvent(&authz.EventGrant{
		MsgTypeUrl: authorization.MsgTypeURL(),
		Granter:    granter.String(),
		Grantee:    grantee.String(),
	})

I don't see anything special about encoding or anything like that here. So this has nothing to do with x/authz at all. Maybe it's how EmitTypedEvent works since it takes a Proto message? Perhaps it's doing some escaping via Proto JSON?

Do you have a tx hash as an example that I can look at on the Hub?

@aaronc @AmauryM Do you know if this is due to Proto encoding when calling EmitTypedEvent?

@alexanderbez alexanderbez changed the title C:x/authz uses extra " around value fileds in events for grant transactions EmitTypedEvent Escapes Strings Jul 17, 2022
@alexpGH
Copy link
Author

alexpGH commented Jul 18, 2022

You can basically use any grant tx e.g. related to the restake app.
E.g. (arbitrarily picked): 8752467CB7D8DE037858E80D90EC3E31399F118240985F31908E9ED87BD350F7

I'm not too deep in the current cosmos-sdk, but had a look also in the prtotobuf part, compared it to what others do, but could not spot a difference, e.g. the cosmos-sdk/proto/cosmos/authz/v1beta1/ vs. bank/v1beta1.
What I find interesting that I never saw the extra " around any other field for all the txs I had a look at the last days, only authz related (might be that I have seen only a subset).

What i however know is that there is a bug in authz related to JSON representation (however leading to elevate the whole data 1 level, and seems unrelated to me: https://medium.com/confio/authz-and-ledger-signing-an-executive-summary-8754a0dc2a88)

@amaury1093
Copy link
Contributor

That's the way how it's done now: in attribute={value}, value can be anything that's JSON-encodable. For strings, JSON-encoded strings have " around them. See #7762 (comment) for details, or code.

The proposed solution back then was to have a SDK engine for querying events (maybe even store them in state, or not, TBD). But for now, I guess if you want to use TM to query txs, you would need to query with "

@alexpGH
Copy link
Author

alexpGH commented Jul 18, 2022

Ok, so the point from #7762 (comment) is
However, strings will have "" around them, and I'm proposing to make search events work with "".
I checked with the current main branch head, simd allows to use " in query txs --events.
However current recommended versions for e.g. cosmoshub (and most others) do not allow " in the query. At least I was not able to somehow mask the extra " such that it went through. (Is there a possibility I overlooked?)

So we are now left with the situation that the authz transactions can not be queried based on garnter grantee fields for some time.

@alexanderbez
Copy link
Contributor

I recall there was a PR recently to event querying to allow = characters too, I wonder if this will allow " escaping? Have you tried on main?

@alexpGH
Copy link
Author

alexpGH commented Jul 19, 2022

As stated above:

I checked with the current main branch head, simd allows to use " in query txs --events.

so it works with newer versions of the daemons, these are however not yet listed in the chain registry as recommended/compatible versions.

I wonder why do the other events still emit without "? Will this change after update or will this only change for newer modules / modules actively altered for this?

@amaury1093
Copy link
Contributor

I wonder why do the other events still emit without "? Will this change after update or will this only change for newer modules / modules actively altered for this?

That's because only some modules use EmitTypedEvent (authz and feegrant only I think), all other modules use normal events, i.e. EmitEvent. That's an inconsistency in the SDK for now, unfortunately.

@tac0turtle
Copy link
Member

Im not sure if there is an actionable as part of this issue other than migrate more modules to use typed events. We also have started emitting our own events with the adoption of adr 38. users can use that to avoid some tendermint related issues, closing this issue, since the migration of more modules to use typed events is captured in an other issue

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

No branches or pull requests

4 participants