-
Notifications
You must be signed in to change notification settings - Fork 83
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
fix: initialize involved accounts addresses to empty slice instead of nil #110
fix: initialize involved accounts addresses to empty slice instead of nil #110
Conversation
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.
💯
@dadamu Hey actually we shouldn't store an empty value for |
@MonikaCat The creator field can be known only if we register The empty involved accounts addresses is happening since Jackal is using Cosmos-SDK v0.45.x version, it would be at least signer event and solve the issue when they upgrade to ^0.46.x, reference here. As a result, I would consider this is the Jackal chain bug that doesn't register a proper event attribute to include at least signer address for the messages. There is even no way to get the transactions by address with the query In order to include the chain who do not emit events properly, I think it would be better to allow involve account addresses to an empty array. What do you think? |
@dadamu right I see, but in this case we will not be able to query and display those txs in account page - it's going to look like some txs are missing, how about we keep parsing the msgs using the codec just for jackal until they upgrade to cosmos-sdk v0.46.x ? What do you think about it? |
@MonikaCat It is fine that we cannot link those txs to account. Currently on Jackal no one can get transactions by given account address because they didn't set event properly. If we keep codec to parse messages, we need to maintain several versions for Juno and Callisto, I don't think it is worth to remain Codec because it is wasting our resource. |
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.
utACK
2ef70b8
to
0847666
Compare
… nil (forbole#110) <!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! ✰ v Before smashing the submit button please review the checkboxes. v If a checkbox is n/a - please still include it but + a little note why ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> ## Description <!-- Small description --> Currently, our messages table has a field called `involved_accounts_addresses` which is parsed from `tx.events` and it does not allow `NULL` value. However, it might be a nil slice when a transaction does not include the event related to addresses, so error messages could happen: ```bash ERR error while handling message err="pq: null value in column \"involved_accounts_addresses\" of relation \"message_68\" violates not-null constraint" height=6808106 module=messages msg_type=/canine_chain.storage.MsgRequestReportForm tx_hash=0F587A0BE501C2DFCA092A5640D3652179567FECFEBE4B98340FA6BB4D3F25DA ``` References: https://bigdipper.live/jackal/transactions/0F587A0BE501C2DFCA092A5640D3652179567FECFEBE4B98340FA6BB4D3F25DA ## Checklist - [x] Targeted PR against correct branch. - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. - [ ] Wrote unit tests. - [x] Re-reviewed `Files changed` in the Github PR explorer.
Description
Currently, our messages table has a field called
involved_accounts_addresses
which is parsed fromtx.events
and it does not allowNULL
value. However, it might be a nil slice when a transaction does not include the event related to addresses, so error messages could happen:References:
https://bigdipper.live/jackal/transactions/0F587A0BE501C2DFCA092A5640D3652179567FECFEBE4B98340FA6BB4D3F25DA
Checklist
Files changed
in the Github PR explorer.