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

gfanton · 2023-12-13T10:48:02Z

This PR implements the strategy discussed in Issue #1174 on the gnolang/gno repository and moves the tm2/keys/client package to gno.land/pkg/keyscli. The main goal is to make tm2 completely unaware of gnovm and gno.land.

This will be a two-step process: starting with the removal of the client from tm2, and later maximizing shared code as needed.

EDIT: it will also enable golang workspace setup: preview

BREAKING CHANGE: client package is now moved inside gno.land/pkg/keyscli

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-13T10:51:59Z

Codecov Report

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

Comparison is base (29fd2ea) 55.85% compared to head (150c8d2) 47.07%.

Files	Patch %	Lines
tm2/pkg/crypto/keys/client/maketx.go	0.00%	91 Missing ⚠️
tm2/pkg/crypto/keys/client/send.go	0.00%	20 Missing ⚠️
tm2/pkg/crypto/keys/client/add.go	40.00%	16 Missing and 2 partials ⚠️
tm2/pkg/crypto/keys/client/sign.go	44.82%	13 Missing and 3 partials ⚠️
tm2/pkg/crypto/keys/client/query.go	0.00%	14 Missing ⚠️
tm2/pkg/crypto/keys/client/root.go	0.00%	13 Missing ⚠️
tm2/pkg/crypto/keys/client/broadcast.go	0.00%	12 Missing ⚠️
tm2/pkg/crypto/keys/client/import.go	52.63%	9 Missing ⚠️
tm2/pkg/crypto/keys/client/export.go	52.94%	8 Missing ⚠️
tm2/pkg/crypto/keys/client/delete.go	36.36%	7 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1438      +/-   ##
==========================================
- Coverage   55.85%   47.07%   -8.79%     
==========================================
  Files         431      366      -65     
  Lines       65729    60615    -5114     
==========================================
- Hits        36713    28534    -8179     
- Misses      26140    29727    +3587     
+ Partials     2876     2354     -522

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.

moul · 2023-12-13T14:23:24Z

Please add a README.md file in the tm2/crypto/keys/client/ folder to explain that this folder is currently not in use anymore but will be in the future again, once we have gained experience with the package. Include a link to this PR and the related issue. As a bonus, consider adding a .go file that raises a deprecation error with the new specified path if someone attempts to import the old path.

deelawn

looks good to me 🥇

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

moul · 2023-12-15T08:35:34Z

Please open an issue before or after merging so that we can track the creation of a new generic later.

moul · 2023-12-16T11:45:57Z

A first generic part that could be moved/kept in tm2 is the low-level query/message engine.

It can easily be done in iterations; and also make keys/client (CLI) importing another library that is purely the low level client but not managing any password reading for instance.

zivkovicmilos

Love it 🚀

@gfanton Please add in the description of the PR that this PR contains breaking functionality 🙏

zivkovicmilos · 2023-12-18T15:54:39Z

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

+func NewRootCmd(io commands.IO) *commands.Command {
+	panic("NewRootCmd: has been deprecated, use `gno.land/pkg/keycli` instead")
+}


Do you think we should drop this entirely instead of leaving the API?

For me, leaving a portion of the API here will ultimately lead to either duplicate code or the necessity to expose parts of the client that were not intended to be exposed.
Personally, I would prefer relocating this specific client to gno.land (so it's not removed, just moved elsewhere) and exposing a more generic library here. This library could serve as a versatile tool that can power gnokey or any other tool, not limited to just CLI tools.

We should avoid using deprecated methods and instead opt for a compilation-time approach.

Something like this: https://github.com/quic-go/quic-go/pull/3364/files

@gfanton attempted it and shared a screenshot of the outcome, which unfortunately didn't meet our expectations.

Let's proceed by removing the .go file and retaining only the README.md file; this approach should suffice.

It kind of works, but the issue is that it will leave a permanent error in the code, not just on import. This is especially problematic for tools like golangci-lint and gopls, so it's not suitable for us.

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

thehowl · 2023-12-21T17:30:10Z

So, way forward agreed with Jae:

Make most of the unexported functions and types in tm2/crypto/keys/client exported
Have cmd/gnokey import crypto/keys/client essentially copy the code for the client from root.go -- but then modify maketx to have its own message types.
Code for its own message types pertains to cmd/gnokey (or a substitute helper package, like pkg/keyscli)

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

Closing in favor of #1483

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

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 3 commits December 12, 2023 18:03

fix: move tm2/pkg/keys/client to gno.land/pkg/keyscmd

972d84d

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

chore: fixup keyscmd import

ea8f1a7

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

chore: rename keyscmd to keyscli

007678d

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

gfanton self-assigned this Dec 13, 2023

gfanton requested review from jaekwon, moul and a team as code owners December 13, 2023 10:48

github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Dec 13, 2023

gfanton changed the title ~~feat: move tm2/keys/client in gno.land/pkg/keyscli~~ feat: move tm2/keys/client into gno.land/pkg/keyscli Dec 13, 2023

deelawn approved these changes Dec 13, 2023

View reviewed changes

fix: add some information about the deprecated client

6daf287

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

github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Dec 13, 2023

gfanton added 2 commits December 14, 2023 11:13

chore: small readme update

e71b3c6

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

chore: fixup comment

e6478c3

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

piux2 mentioned this pull request Dec 14, 2023

Minutes: GitHub Review Meetings [every Thursday] gnolang/meetings#18

Open

3 tasks

fix: add build constraint on keys/client

30b0f66

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

moul approved these changes Dec 15, 2023

View reviewed changes

ajnavarro approved these changes Dec 18, 2023

View reviewed changes

zivkovicmilos approved these changes Dec 18, 2023

View reviewed changes

zivkovicmilos added the breaking change Functionality that contains breaking changes label Dec 18, 2023

gfanton changed the title ~~feat: move tm2/keys/client into gno.land/pkg/keyscli~~ BREAKING CHANGE: move tm2/keys/client into gno.land/pkg/keyscli Dec 18, 2023

Merge branch 'master' into feat/extract-tm2-depencies

173978b

ajnavarro changed the title ~~BREAKING CHANGE: move tm2/keys/client into gno.land/pkg/keyscli~~ refactor: move tm2/keys/client into gno.land/pkg/keyscli Dec 18, 2023

zivkovicmilos mentioned this pull request Dec 20, 2023

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

Open

fix: remove all method signature in keys/client

68dc091

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

thehowl changed the title ~~refactor: move tm2/keys/client into gno.land/pkg/keyscli~~ refactor!: move tm2/keys/client into gno.land/pkg/keyscli Dec 21, 2023

thehowl mentioned this pull request Dec 21, 2023

gno [-watch] dev PATH #1174

Closed

Merge branch 'master' into feat/extract-tm2-depencies

243b1f1

gfanton force-pushed the feat/extract-tm2-depencies branch 2 times, most recently from 150c8d2 to 243b1f1 Compare December 21, 2023 23:26

gfanton mentioned this pull request Dec 21, 2023

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

Merged

7 tasks

gfanton closed this Dec 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gfanton commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

moul commented Dec 13, 2023

deelawn left a comment

moul commented Dec 15, 2023

moul commented Dec 16, 2023

zivkovicmilos left a comment •

edited

Loading

zivkovicmilos Dec 18, 2023

gfanton Dec 19, 2023

moul Dec 19, 2023

moul Dec 19, 2023

moul Dec 19, 2023

gfanton Dec 20, 2023

thehowl commented Dec 21, 2023 •

edited

Loading

gfanton commented Dec 21, 2023

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

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

Conversation

gfanton commented Dec 13, 2023 • edited Loading

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

moul commented Dec 13, 2023

deelawn left a comment

Choose a reason for hiding this comment

moul commented Dec 15, 2023

moul commented Dec 16, 2023

zivkovicmilos left a comment • edited Loading

Choose a reason for hiding this comment

zivkovicmilos Dec 18, 2023

Choose a reason for hiding this comment

gfanton Dec 19, 2023

Choose a reason for hiding this comment

moul Dec 19, 2023

Choose a reason for hiding this comment

moul Dec 19, 2023

Choose a reason for hiding this comment

moul Dec 19, 2023

Choose a reason for hiding this comment

gfanton Dec 20, 2023

Choose a reason for hiding this comment

thehowl commented Dec 21, 2023 • edited Loading

gfanton commented Dec 21, 2023

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

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

gfanton commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

zivkovicmilos left a comment •

edited

Loading

thehowl commented Dec 21, 2023 •

edited

Loading