feat(keeper): implement json primitive return #2949

gfanton · 2024-10-14T15:08:31Z

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:

PrimitiveKind -> raw value
Error/Stringer -> execute and return result
~~Object -> <oid:type>~~ <obj:TYPE:OID> as string
Other -> <type> as string

This 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

Probably more tests
Need approval on how we represent object

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

codecov · 2024-10-14T15:11:05Z

Codecov Report

Attention: Patch coverage is 78.09524% with 69 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gno.land/pkg/keyscli/query_json.go	65.51%	15 Missing and 5 partials ⚠️
gno.land/pkg/sdk/vm/handler.go	61.53%	14 Missing and 1 partial ⚠️
gno.land/pkg/sdk/vm/msgs.go	44.00%	14 Missing ⚠️
gno.land/pkg/sdk/vm/convert.go	91.15%	10 Missing ⚠️
gno.land/pkg/sdk/vm/keeper.go	85.29%	9 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

thehowl

overall approach LGTM, let's roll with only supporting primitive types for now

gno.land/pkg/sdk/vm/keeper.go

gno.land/pkg/sdk/vm/msgs.go

gnovm/pkg/gnolang/values.go

tm2/pkg/bft/abci/types/types.go

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gno.land/pkg/sdk/vm/handler.go

moul · 2024-11-14T16:22:09Z

gno.land/pkg/sdk/vm/handler.go

+	QueryRender   = "qrender"
+	QueryFuncs    = "qfuncs"
+	QueryEval     = "qeval"
+	QueryEvalJSON = "qeval/json" // EXPERIMENTAL


I think I prefer a single call with a flag to specify the format.
Any other opinions?

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?

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.

@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.

gno.land/pkg/sdk/vm/keeper.go

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

moul · 2025-01-27T11:05:53Z

Can you add a small txtar to illustrate the expected UX?

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

moul · 2025-01-28T10:07:44Z

gno.land/pkg/gnoland/app.go

-			fmt.Sprintf("%s(%d)", valChangesFn, app.LastBlockHeight()),
-		)
+		expr := fmt.Sprintf("%s(%d)", valChangesFn, app.LastBlockHeight())
+		msgEval := vm.NewMsgEval(vm.FormatDefault, valRealm, expr)


@zivkovicmilos, we don't need to port this module in this PR, but I'd like your opinion on what the diff will look like thanks to the new JSON built-in support.

moul · 2025-01-28T10:09:01Z

gno.land/pkg/keyscli/query_json.go

+	)
+}
+
+func execQuery(cfg *client.QueryCfg, args []string, io commands.IO) error {


Suggested change

func execQuery(cfg *client.QueryCfg, args []string, io commands.IO) error {

func execJSONQuery(cfg *client.QueryCfg, args []string, io commands.IO) error {

moul · 2025-01-28T10:10:02Z

gno.land/pkg/keyscli/query_json.go

+	}
+
+	// XXX: this is probably too specific
+	if cfg.Path == "vm/qeval/json" {


Shouldn't this be a preliminary check raising an error at the beginning of the function?

moul · 2025-01-28T10:12:55Z

gnovm/pkg/gnolang/values_string.go

+	return IsImplementedBy(gStringerType, tv.T)
+}
+
+func (tv *TypedValue) IsError() bool {


I'm not sure that this check is universally seen as the way to indicate that a TypedValue is an error. It might make sense to rename it to ImplError(). Thoughts?

Hmm, while IsStringer makes more sense since Stringer is universal for the String() implementation, I agree that IsError is not obvious and can be confusing. I can rename it to ImplError, but if I do that, I should also rename IsStringer to ImplStringer for consistency.

moul · 2025-01-28T10:15:03Z

gno.land/pkg/sdk/vm/handler.go

+	case MsgEval:
+		return vh.handleMsgEval(ctx, msg)


Suggested change

case MsgEval:

return vh.handleMsgEval(ctx, msg)

Having a MsgEval struct instead of using arguments may be sensible. However, I believe we should not register Eval as a possible transaction type in that manner.

moul · 2025-01-28T10:17:05Z

gno.land/pkg/sdk/vm/msgs.go

+
+var _ std.Msg = MsgEval{}
+
+func NewMsgEval(format Format, pkgPath, expr string) MsgEval {


Suggested change

func NewMsgEval(format Format, pkgPath, expr string) MsgEval {

func NewMsgEval(pkgPath, expr string, format Format) MsgEval {

Personal preference for this order.

the more i think about it, the more i'm for having such struct, but unexported, without New constructor, not in the msgs.go file.

and probably centralizing everything about the query in query.go.

moul · 2025-01-28T10:17:45Z

gno.land/pkg/sdk/vm/msgs.go

+func (msg MsgEval) GetSignBytes() []byte {
+	return std.MustSortJSON(amino.MustMarshalJSON(msg))


I support removing this and preventing MsgCall from being included in a transaction. It's just a query.

moul · 2025-01-28T10:19:42Z

gno.land/pkg/sdk/vm/keeper.go

@@ -46,7 +47,7 @@ const (
 type VMKeeperI interface {
 	AddPackage(ctx sdk.Context, msg MsgAddPackage) error
 	Call(ctx sdk.Context, msg MsgCall) (res string, err error)
-	QueryEval(ctx sdk.Context, pkgPath string, expr string) (res string, err error)
+	Eval(ctx sdk.Context, msg MsgEval) (res string, err error)


Suggested change

Eval(ctx sdk.Context, msg MsgEval) (res string, err error)

QueryEval(ctx sdk.Context, pkgPath, expr string, format Format) (res string, err error)

What about this?

Suggested change

Eval(ctx sdk.Context, msg MsgEval) (res string, err error)

QueryEval(ctx sdk.Context, cfg EvalCfg) (res string, err error)

To support optional flags, this is probably better.

moul · 2025-01-28T10:25:18Z

gno.land/pkg/sdk/vm/keeper.go

 		}
-		return res, nil
+


can't you add the two \n here and remove the if condition here: https://github.com/gnolang/gno/pull/2949/files#diff-3663e30f0c0e01d8d47bb7974d605b4cf12d1a244970c6a9bbba55fb0be6acedR503 ?

As only Call can handle multiple msgs, I don't think we should do that; we don't want Queries to end with a useless \n in their response.

moul · 2025-01-28T10:26:24Z

gno.land/pkg/sdk/vm/msgs.go

+const (
+	FormatMachine Format = "machine" // Default machine representation
+	FormatString         = "string"  // Single string representation
+	FormatJSON           = "json"    // XXX: EXPERIMENTAL, only supports primitive types for now


Suggested change

FormatJSON = "json" // XXX: EXPERIMENTAL, only supports primitive types for now

FormatJSON = "json" // XXX: EXPERIMENTAL, only supports primitive types for now

FormatJSONP = "jsonp" // XXX: EXPERIMENTAL, only supports primitive types for now

I'm for adding right now the jsonp alternative. Btw jsonp takes an extra argument (the callback name).

moul · 2025-01-28T10:27:11Z

gno.land/pkg/sdk/vm/msgs.go

+	// XXX: This field is experimental, use with care as output is likely to change
+	// Default format is machine
+	Format Format `json:"format" yaml:"format"`
+}


Suggested change

}

FormatOpts string `json:"format-opts" yaml:"format-opts"`

}

to support formatting options, such as JSONP' callback, and eventually other things later?

moul · 2025-01-28T10:28:22Z

gno.land/pkg/sdk/vm/package.go

@@ -14,6 +14,7 @@ var Package = amino.RegisterPackage(amino.NewPackage(
 	std.Package,
 	gnovm.Package,
 ).WithTypes(
+	MsgEval{}, "m_eval",


Suggested change

MsgEval{}, "m_eval",

moul · 2025-01-28T10:28:48Z

gno.land/pkg/keyscli/query_json.go

+	cfg.Path = args[0]
+	if cfg.Path == "vm/qeval" {
+		// automatically add json suffix for qeval
+		cfg.Path = cfg.Path + "/json"


looks weird. what's the goal? maybe add a comment if there is a valid reason.

Will do. The reason for that is this jquery is made to handle JSON, so you want your query to return JSON. Therefore, you should not have to specify the format for the query. The goal is to automatically pass the argument to produce a JSON output without having to specify it every time.
query vm/qeval abc.xy/r/toto -> machine
jquery vm/qeval abc.xy/r/toto -> JSON

moul · 2025-01-28T10:31:14Z

gno.land/pkg/sdk/vm/convert.go

+			slice := tv.V.(*gno.SliceValue)
+			if data := slice.GetBase(nil).Data; data != nil {
+				i := slice.GetLength()
+				return `"` + base64.StdEncoding.EncodeToString(data[:i]) + `"`


can't we support slices and arrays just like this:

"[" + strings.Join(JSONPrimitiveValues(vals), ",") + "]"?

It would be cool, but not like this, as it would not handle cyclic dependencies. Setting a depth could be a solution, but I prefer handling this in a separate PR.

moul · 2025-01-28T10:32:35Z

gno.land/pkg/sdk/vm/keeper.go

+	return res, nil
+}
+
+func stringifyResultValues(m *gno.Machine, format Format, values []gno.TypedValue) string {


implement or add a comment to add gasmeter here.

As this method inherits the gno.Machine, I think it naturally handles gas here.
@piux2 can you confirm ?

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

thehowl · 2025-01-29T18:24:56Z

gno.land/pkg/keyscli/query_json.go

+
+	return commands.NewCommand(
+		commands.Metadata{
+			Name:       "jquery",


This name gives me shivers. Could we not do gnokey query --json? Seems also easier to support in broadcast and maketx if it's just a flag.

What case do you see with broadcast/maketx and JSON?

I personally like "jquery"; it sounds like "anti-web 2.0." :)

thehowl · 2025-01-29T18:36:50Z

gno.land/pkg/sdk/vm/convert.go

+	}
+
+	// Fallback on generic object handler
+	if pv, ok := tv.V.(gno.PointerValue); ok {


Could we not also consider the case where tv.V is an Object?

switch v := tv.V.(type) { case *PointerValue: case Object: default: }

gfanton added 4 commits September 27, 2024 11:00

feat: json primitive

7821510

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

feat: json primitive

11c0327

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

fix: test

44de899

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

wip: json primitive next

aca85fb

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

github-actions bot assigned gfanton Oct 14, 2024

github-actions bot added 🧾 package/realm Tag used for new Realms or Packages. 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Oct 14, 2024

thehowl reviewed Oct 17, 2024

View reviewed changes

gno.land/pkg/sdk/vm/keeper.go Outdated Show resolved Hide resolved

gno.land/pkg/sdk/vm/msgs.go Outdated Show resolved Hide resolved

gnovm/pkg/gnolang/values.go Outdated Show resolved Hide resolved

tm2/pkg/bft/abci/types/types.go Outdated Show resolved Hide resolved

Kouteki mentioned this pull request Oct 18, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

thehowl mentioned this pull request Oct 22, 2024

RPC call to view realm state #2996

Open

gfanton added 4 commits November 13, 2024 11:06

Merge remote-tracking branch 'origin/master' into feat/json-primitive

dd00a47

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

wip: add format to query eval

6347f25

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

feat: add msg eval

e906187

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

chore: cleanup

cf7e6d0

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

moul reviewed Nov 14, 2024

View reviewed changes

gno.land/pkg/sdk/vm/handler.go Outdated Show resolved Hide resolved

moul reviewed Nov 14, 2024

View reviewed changes

gno.land/pkg/sdk/vm/keeper.go Outdated Show resolved Hide resolved

moul reviewed Nov 14, 2024

View reviewed changes

gno.land/pkg/sdk/vm/keeper.go Show resolved Hide resolved

moul reviewed Nov 14, 2024

View reviewed changes

gno.land/pkg/sdk/vm/keeper.go Outdated Show resolved Hide resolved

Kouteki added the in focus Core team is prioritizing this work label Nov 15, 2024

Kouteki added this to the 🚀 Mainnet launch milestone Nov 22, 2024

Kouteki mentioned this pull request Nov 22, 2024

[Keeper] JSON - Gas Consumption #1860

Open

thehowl mentioned this pull request Nov 27, 2024

PoC: VMKeeper.QueryEvalJSON #444

Closed

gfanton added 4 commits January 8, 2025 00:15

Merge remote-tracking branch 'origin/master' into feat/json-primitive

a9d0af8

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

wip: use Eval keeper function for query

e5d26e1

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

feat: add jquery to gnokey

424308a

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

chore: doc

9aa7139

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

fix: msg eval format

4db0338

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

moul mentioned this pull request Jan 18, 2025

[gnoweb] Design System Overhaul & Markdown Extensions #3255

Open

gfanton added 3 commits January 24, 2025 17:16

Merge remote-tracking branch 'origin/master' into feat/json-primitive

eb85c50

fix: test

d215c18

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

fix: test pt2

35ed3d9

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton changed the title ~~wip(keeper): implement json primitive return~~ feat(keeper): implement json primitive return Jan 26, 2025

chore: cleanup

6a47f6a

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton marked this pull request as ready for review January 26, 2025 22:07

gfanton added 2 commits January 26, 2025 23:09

chore: cleanup

ceb0f3b

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

fix: object id repr

a3a2ce6

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton added 2 commits January 27, 2025 15:58

fix: do not encode < and > in jquery command

ce91f7e

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

feat: add jquery test

b251235

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

moul reviewed Jan 28, 2025

View reviewed changes

chore: typo

16d496d

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

thehowl reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(keeper): implement json primitive return #2949

feat(keeper): implement json primitive return #2949

gfanton commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

thehowl left a comment

moul Nov 14, 2024

gfanton Jan 8, 2025

gfanton Jan 8, 2025 •

edited

Loading

gfanton Jan 9, 2025 •

edited

Loading

moul commented Jan 27, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

gfanton Jan 29, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

gfanton Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

moul Jan 28, 2025

gfanton Jan 28, 2025

moul Jan 28, 2025

gfanton Jan 28, 2025

moul Jan 28, 2025

gfanton Jan 28, 2025

thehowl Jan 29, 2025

moul Jan 31, 2025

thehowl Jan 29, 2025

	func execQuery(cfg *client.QueryCfg, args []string, io commands.IO) error {
	func execJSONQuery(cfg *client.QueryCfg, args []string, io commands.IO) error {


		var _ std.Msg = MsgEval{}

		func NewMsgEval(format Format, pkgPath, expr string) MsgEval {

	func NewMsgEval(format Format, pkgPath, expr string) MsgEval {
	func NewMsgEval(pkgPath, expr string, format Format) MsgEval {

		func (msg MsgEval) GetSignBytes() []byte {
		return std.MustSortJSON(amino.MustMarshalJSON(msg))

	Eval(ctx sdk.Context, msg MsgEval) (res string, err error)
	QueryEval(ctx sdk.Context, pkgPath, expr string, format Format) (res string, err error)

	Eval(ctx sdk.Context, msg MsgEval) (res string, err error)
	QueryEval(ctx sdk.Context, cfg EvalCfg) (res string, err error)

	FormatJSON = "json" // XXX: EXPERIMENTAL, only supports primitive types for now
	FormatJSON = "json" // XXX: EXPERIMENTAL, only supports primitive types for now
	FormatJSONP = "jsonp" // XXX: EXPERIMENTAL, only supports primitive types for now

feat(keeper): implement json primitive return #2949

Are you sure you want to change the base?

feat(keeper): implement json primitive return #2949

Conversation

gfanton commented Oct 14, 2024 • edited Loading

TODOs

codecov bot commented Oct 14, 2024 • edited Loading

Codecov Report

thehowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfanton Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

gfanton Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

moul commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gfanton commented Oct 14, 2024 •

edited

Loading

codecov bot commented Oct 14, 2024 •

edited

Loading

gfanton Jan 8, 2025 •

edited

Loading

gfanton Jan 9, 2025 •

edited

Loading