-
Notifications
You must be signed in to change notification settings - Fork 386
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
wip(keeper): implement json primitive return #2949
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
overall approach LGTM, let's roll with only supporting primitive types for now
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
gno.land/pkg/sdk/vm/handler.go
Outdated
QueryRender = "qrender" | ||
QueryFuncs = "qfuncs" | ||
QueryEval = "qeval" | ||
QueryEvalJSON = "qeval/json" // EXPERIMENTAL |
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 I prefer a single call with a flag to specify the format.
Any other opinions?
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.
A solution could be to have "qeval" data
as a marshaled MsgEval
, but that would break the current behavior of qeval
until the current behavior is set as a fallback if unmarshal MsgEval
fail.
I'm hesitating; it would probably give more flexibility, but qeval/json
is really more straightforward.
wdyt?
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.
Ok. After some attempts with url.Url
using a query to parse the JSON flag or employing \n
as a flag separator, I found it problematic and difficult to escape. This issue arises when users include ?
or \n
within the expression's arguments. In my opinion, we should use qeval/json
as it looks like the most straightforward method to handle this for now, given the current implementation of the handler.
A possible compromise is to check the json
part of vm/qeval/json
within the queryEval
function, allowing the right part of the path to serve as a flag.
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.
@moul I've ended up going with the path query flag; see the queryEval
function. We can probably set another separator than /
to distinguish between the specific path and the flag. However, having this as a distinct sub-path does especially chock me.
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Yet another JSON PR 🎉
This is a simpler implementation of the JSON keeper result that introduces JSON primitive results in keeper MsgCall and MsgQuery result. To maintain backward compatibility, it's not enabled by default, and you have to specify the format in the request.
The result will be returned in the form of a parsable JSON array.
Currently, it will resolve types as follows:
<oid:type>
as string<type>
as stringThis PR also adds a small utility command in gnokey called
jquery
that outputs a parsable JSON response. This command is flagged as experimental, as are all JSON features present in the handler/keeper.TODOs
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description