Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 scvals to print in json string #305
Fix scvals to print in json string #305
Changes from 4 commits
776e9ad
03a2fe2
27582a7
bf53769
176f8c5
9a10940
f9c3f6b
7cfef0a
0141220
ed0f744
73a1246
5df0a03
1d43e8a
0c80a17
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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 is the xdr2json. It works pretty well and will now output like
How does this look now @sydneynotthecity @leighmcculloch
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.
yeah i like this better than cli. i don't like that we've copied over
xdr2json
and think that that should be published as its own package but that's outside the scope of this work.do you think there is merit in us using xdr2json to parse operation details in the future? this looks much simpler to parse vs what we have to do today
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.
I think it depends. For the nested params (also ScVals) for invoke host function yes this makes sense. For some of the other operations where we do some mafhs or add extra non-xdr labels maybe 🤷♀️
I think we could technically redesign the operations.Details field as a whole too.
Definitely worth a spike though
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.
Why does the value have two layers of JSON? The outer layer doesn't look like the canonical xdr-json format.
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.
The outer layer is not. There's a few things that need to be reworked (mentioned it should be put back in draft).
I think it's debatable whether or not to remove that outer layer or not.
Pros with the extra Type/Value outer layer
ArmForSwitch(ScVal.Type)
strings rather than the string names from xdr2json (e.g.,Instance
vscontract_instance
). I don't know if there is an existing list of the type names like in the xdr const or ArmForSwitch()topics_decoded
but the JSON inside also starts withtopics_decoded
meaning to access it you have toselect topics_decoded.topics_decoded.<the json field you want>
Cons
One thing I should do is I think we can save it as a JSON instead of string(json). This will depend on how many schema changes and if it's okay to change to a generic interface{}.
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.
FYI the json isn't stable between protocol versions. I mean it won't change unnecessarily, but xdr structure can change in ways that is binary backwards compatible but not structurally backwards compatible.
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.
Would be helpful to get some feedback about the structure of the json and what you'd change if anything.
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.
That's unfortunate but makes sense (aka unified event changes)
Yup I'll revise and update with the proposed structure on Monday for y'all's review. I'll add some examples SQL queries to show what it'll look like in BQ as well
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.
Thinking and playing with the ScVal column in BQ I think it makes sense to just make everything a generic interface (use whatever xdr2json spits out). The main reason I liked the outer layer was for easier filtering BUT I think that filtering ScVal types will be rare but still possible in BQ. And if it is a pain this change could be added downstream (dbt model for enriched contract events) keeping the
history_contract_events
table at its rawest form.Another benefit is the human names for the ScVal types from xdr2json are nicer than the
ArmForSwitch
. Not sure why they diverged but I would say it'd be easy enough to make a new map/const/enum for the xdr2json names if needed (or I wonder if we can update ArmForSwitch).Example traversing column in BQ is still the same except with the xdr2json names
Example filtering in BQ
Original filtering in BQ
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.
There's a few things that the Rust xdr lib does that we didn't implement in other xdr libs: