feat: remove `gnovm` and `gno.land` dependencies from tm2 #1483

gfanton · 2023-12-21T23:46:04Z

This PR shares the same goal as #1438: to make tm2 completely unaware of gnovm and gno.land. However, it adopts a different strategy to achieve this, following #1438 (comment). It will:

Exposed almost everything from the tm2/crypto/keys/client package:
- NewCommandXXX, along with their specific structure configurations and fields, are now exposed to allow the creation of composable CLI tools.
- XXXHandler (including Sign, Verify, Broadcast, etc.) are now exposed to enable external packages to reuse some logic from the keys/client package.
Moved specific gnovm/gno.land commands to the gnokey package: run, call, and addpkg.
In an effort to avoid duplicate code, an ExecSignAndBroadcast method has been exposed. However, it appears too specific, and I am considering duplicating it regardless. I would appreciate thoughts on this.

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 · 2023-12-21T23:48:00Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (82c89ce) 55.85% compared to head (cc506ef) 55.91%.

Files	Patch %	Lines
gno.land/cmd/gnokey/main.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1483      +/-   ##
==========================================
+ Coverage   55.85%   55.91%   +0.06%     
==========================================
  Files         431      431              
  Lines       65839    65684     -155     
==========================================
- Hits        36773    36727      -46     
+ Misses      26185    26081     -104     
+ Partials     2881     2876       -5

Flag	Coverage Δ
go-1.21.x	`∅ <ø> (∅)`
misc	`∅ <ø> (∅)`
misc-_test.genstd	`∅ <ø> (∅)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

moul · 2023-12-22T10:41:57Z

gno.land/pkg/keyscli/addpkg.go

+// TODO: move most of the logic in ROOT/gno.land/...
+


Suggested change

// TODO: move most of the logic in ROOT/gno.land/...

moul · 2023-12-22T10:42:30Z

gno.land/pkg/keyscli/addpkg.go

@@ -0,0 +1,140 @@
+package keyscli


please, add a small readme explaining the goal of this package and its relationships and add a link to this PR.

moul · 2023-12-22T10:43:45Z

gno.land/pkg/keyscli/run.go


 import (
 	"context"
 	"flag"
 	"fmt"
-	"io/ioutil"
+	ioutil "io"


Suggested change

ioutil "io"

"io"

I didn't use io because it will make me rename 'io' argument from commands.IO

do rename that one ;)

moul · 2023-12-22T10:46:06Z

tm2/pkg/crypto/keys/client/addpkg.go

@@ -1,222 +0,0 @@
-package client


The file is marked as deleted, but it was actually moved and then modified. We want to preserve the file's history. To handle the rename and edit, we can make two commits or explore other options. cc @harry-hov.

hmm, from an history point view, it's correctly mark as renamed in the commit: 32511b3
but since this PR will be squash when merge, im not sure we can do anything about it...

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

thehowl · 2024-01-07T21:21:24Z

🤯 the pr number is an anagram of the old #1438

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

zivkovicmilos

Looks great 💯

Thank you for going through with the split 🙏

In an effort to avoid duplicate code, an ExecSignAndBroadcast method has been exposed. However, it appears too specific, and I am considering duplicating it regardless. I would appreciate thoughts on this.

This is honestly one of the sketchier parts of gnokey, the entire sign / broadcast pipeline. I like the approach you've gone with for now, I think it's out of the scope of this PR to fix these flows (that affect a good chunk of gnokey commands).

tm2/pkg/crypto/keys/client/maketx.go

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

This PR shares the same goal as gnolang#1438: to make `tm2` completely unaware of `gnovm` and `gno.land`. However, it adopts a different strategy to achieve this, following gnolang#1438 (comment). It will: - Exposed almost everything from the `tm2/crypto/keys/client` package: - `NewCommandXXX`, along with their specific structure configurations and fields, are now exposed to allow the creation of composable CLI tools. - `XXXHandler` (including Sign, Verify, Broadcast, etc.) are now exposed to enable external packages to reuse some logic from the `keys/client` package. - Moved specific `gnovm`/`gno.land` commands to the `gnokey` package: `run`, `call`, and `addpkg`. - In an effort to avoid duplicate code, an `ExecSignAndBroadcast` method has been exposed. However, it appears too specific, and I am considering duplicating it regardless. I would appreciate thoughts on this. <details><summary>Contributors' checklist...</summary> - [ ] 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](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>

gfanton added 6 commits December 21, 2023 23:12

fix: expose most of the method/fields in keys/client

b222d72

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

feat: add SignAndBroadcastHandler

489e4a4

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

fix: extract gnoland and gnovm outside tm2

32511b3

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

fix: remove deprecated ioutil

9ec535f

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

chore: cleanup root

4e7da28

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

fixup: wrong home config

150c8d2

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

gfanton requested review from jaekwon, moul and a team as code owners December 21, 2023 23:46

github-actions bot assigned gfanton Dec 21, 2023

github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Dec 21, 2023

gfanton mentioned this pull request Dec 21, 2023

refactor!: move tm2/keys/client into gno.land/pkg/keyscli #1438

Closed

7 tasks

chore: mod tidy

f7aa61f

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

fix: use keyscli module to be able to share package code with test in…

ddbddd2

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

gfanton force-pushed the feat/reworks-keys-client branch from 3b7ab05 to ddbddd2 Compare December 22, 2023 00:34

zivkovicmilos self-requested a review December 22, 2023 10:09

moul reviewed Dec 22, 2023

View reviewed changes

moul approved these changes Dec 22, 2023

View reviewed changes

gfanton added 3 commits January 5, 2024 15:55

chore: add keycli readme

b0cfbbf

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

chore: cleanup

840ba7c

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

chore: use cmdio instead of io

95f16f4

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

Merge branch 'master' into feat/reworks-keys-client

61f6fde

michelleellen mentioned this pull request Jan 10, 2024

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

Merge remote-tracking branch 'master' into feat/reworks-keys-client

cc506ef

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

thehowl mentioned this pull request Jan 12, 2024

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

Open

gfanton added 2 commits January 15, 2024 09:44

Merge remote-tracking branch 'master' into feat/reworks-keys-client

65e16b7

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

chore: lint

13b3af1

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

zivkovicmilos approved these changes Jan 15, 2024

View reviewed changes

tm2/pkg/crypto/keys/client/maketx.go Show resolved Hide resolved

gfanton added 2 commits January 15, 2024 17:18

Merge remote-tracking branch 'master' into feat/reworks-keys-client

589d7e9

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

fix: rebase

6f92d37

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

moul merged commit 5ad12cd into gnolang:master Jan 15, 2024
67 of 71 checks passed

grepsuzette mentioned this pull request Jun 18, 2024

fix(gnokeykc): gnokeykc maketx {addpkg,call,run} missing because of tm2 separation (fixes #2373) #2375

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove `gnovm` and `gno.land` dependencies from tm2 #1483

feat: remove `gnovm` and `gno.land` dependencies from tm2 #1483

gfanton commented Dec 21, 2023

codecov bot commented Dec 21, 2023 •

edited

Loading

moul Dec 22, 2023

moul Dec 22, 2023

moul Dec 22, 2023

gfanton Dec 22, 2023

thehowl Jan 8, 2024

gfanton Jan 8, 2024

moul Dec 22, 2023

gfanton Jan 5, 2024

thehowl commented Jan 7, 2024

zivkovicmilos left a comment

feat: remove gnovm and gno.land dependencies from tm2 #1483

feat: remove gnovm and gno.land dependencies from tm2 #1483

Conversation

gfanton commented Dec 21, 2023

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

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

thehowl commented Jan 7, 2024

zivkovicmilos left a comment

Choose a reason for hiding this comment

feat: remove `gnovm` and `gno.land` dependencies from tm2 #1483

feat: remove `gnovm` and `gno.land` dependencies from tm2 #1483

codecov bot commented Dec 21, 2023 •

edited

Loading