-
Notifications
You must be signed in to change notification settings - Fork 382
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: tm2 clean up #825
WIP: tm2 clean up #825
Conversation
The action flow failed during CI. Once this is approved and merged. We might need to move tm2txsync in the matrix.program from .github/workflows/tm2.yml to .github/workflows/gnoland.yml |
can the vm package move into gnoland instead? |
Agree; I should have done that. Currently, tm2/pkg/crypto/keys/client/call.go and client/addpkg.go depend on the vm sdk module to construct the MsgCall and MsgAddPkg. When we transfer call.go and addpkg.go from tm2 to gno.land, everything should be in place. I have moved the pkg/sdk/vm folder from gnovm to gno.land. As of now, tm2 relies on gno.land instead of gnovm. |
gno.land/Makefile
Outdated
@@ -6,12 +6,13 @@ help: | |||
rundep=go run -modfile ../misc/devdeps/go.mod | |||
|
|||
.PHONY: build | |||
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet | |||
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.tm2txsync |
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.
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.tm2txsync | |
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.gnotxsync |
Should be renamed to gnotxsync
too.
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.
good catch. I updated them all.
Hey @piux2, what's your plan for this initiative?
|
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.gnotxsync | ||
|
||
build.gnoland:; go build -o build/gnoland ./cmd/gnoland | ||
build.gnoweb:; go build -o build/gnoweb ./cmd/gnoweb | ||
build.gnofaucet:; go build -o build/gnofaucet ./cmd/gnofaucet | ||
build.gnokey:; go build -o build/gnokey ./cmd/gnokey | ||
_build.gnotxsync:; go build -o build/gnotxsync ./cmd/gnotxsync |
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.
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet _build.gnotxsync | |
build.gnoland:; go build -o build/gnoland ./cmd/gnoland | |
build.gnoweb:; go build -o build/gnoweb ./cmd/gnoweb | |
build.gnofaucet:; go build -o build/gnofaucet ./cmd/gnofaucet | |
build.gnokey:; go build -o build/gnokey ./cmd/gnokey | |
_build.gnotxsync:; go build -o build/gnotxsync ./cmd/gnotxsync | |
build: build.gnoland build.gnokey build.gnoweb build.gnofaucet build.gnotxsync | |
build.gnoland:; go build -o build/gnoland ./cmd/gnoland | |
build.gnoweb:; go build -o build/gnoweb ./cmd/gnoweb | |
build.gnofaucet:; go build -o build/gnofaucet ./cmd/gnofaucet | |
build.gnokey:; go build -o build/gnokey ./cmd/gnokey | |
build.gnotxsync:; go build -o build/gnotxsync ./cmd/gnotxsync |
@@ -7,12 +7,12 @@ import ( | |||
"flag" | |||
"fmt" | |||
|
|||
"github.com/gnolang/gno/gno.land/pkg/sdk/vm" |
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.
addpkg should be defined in gno.land or gnovm, not tm2.
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.
It appears there's many shared struct between the different commands, so moving only call.go
and addpkg.go
in gno.land/cmd/gnokey
is not straightforward.
What about moving the whole tm2/pkg/crypto/keys/client
folder into gno.land/cmd/gnokey/client
? I tested and it requires only a few changes in some imports:
- "github.com/gnolang/gno/tm2/pkg/crypto/keys/client"
+ "github.com/gnolang/gno/gno.land/cmd/gnokey/client"
tm2/pkg/crypto/keys/client
is not used at all inside tm2
.
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 believe it's a smart approach: let's start by moving to gno.land and then take the time to design well-crafted reusable components in the right place for future blockchains.
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 don't have the right to push to this repo, but I can provide the patch, even if the change is pretty straighforward.
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 tm2/pkg/crypto/keys/client is part of the generic tm2 tool set. We should not remove it from the tm2 folder. The purpose of the separation is that people can create other tools using tm2 packages in addition to gnokey
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.
Not really generic if I may: one of the first thing it does is fetching the config from $GNO_HOME
or $XDG_CONFIG_HOME/gno
.
But I understand your point, it would be better to add the addpkg
and call
subcommands in the gnokey
package. I just feel it's a shame to keep this ultimate dependency.
The other way maybe is to simply turn public the related structs (makeTxCfg
, queryCfg
) and methods (signAndBroadcast
, queryHandler
, ...) . At some point, to make this package more flexible, that makes sense.
@@ -5,11 +5,11 @@ import ( | |||
"flag" | |||
"fmt" | |||
|
|||
"github.com/gnolang/gno/gno.land/pkg/sdk/vm" |
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.
same for call, should be defined in gno.land on gnovm, but not tm2.
Alternatively, we can keep |
As I mentioned in the top discussion section
We can only move them out and create separation if we do either one of the below.
Please checkout the tm2/pkg/crypto/client/root.go and the cfg variable in each command. |
Apologies for the misunderstanding. Let's update this PR to be current and conflict-free, with passing CI. In another PR, we can explore an efficient method to finish the actual split tm2<>gnovm. Thank you! |
should i take a look at this now? |
Jae, no need to check this immediately. But feel free. While I initially thought this PR would finalize the split, it's a preliminary step. I suggest we help/let @piux2 to pass CI and merge conflicts. Then, I'll review recent changes (you pre-approved a long time ago), then merge. Later, we'll seek your review for the new complementary PR. |
Superseded by #1080 |
<!-- please provide a detailed description of the changes made in this pull request. --> <details><summary>Original text from #825</summary> <p> # Description Part of #692 #585 BREAKING CHANGE: In GitHub Action Flow, the build script needs to be updated on moving tm2txsync from tm2/cmd to gno.land/cmd ## gno.land/ It provides information and tools necessary for running gno.land node. It includes - a node application for starting gnoland nodes to deliver an RPC endpoint - gnoweb for web access to the chain. - tools to interact with gno nodes. - a cosmos SDK gno VM module with keeper and handler. In the future, we plan to consolidate tools for node deployment and monitoring. The future plan also includes supporting both IBC and ICS. ## gnovm/ Gnovm is agnostic to any SDK or chain. It is intended for gno contract developers and gno VM developers. It includes - a gno development CLI - gnolang language - virtual machine implementation. - standard libraries for gno contracts, - examples and test cases. Future plans include making gnovm compatible with IBC ## tm2/ Tendermint2 is a BFT consensus engine and a set of tools for blockchain builders More details can be found at [https://github.com/tendermint/tendermint2#readme](https://github.com/tendermint/tendermint2#readme) In the future, tm2 plans to: - Remain synchronized with the tendermint2 repository. - Port issue fixes between Tendermint v0.23.4 and v0.34. - Ensure compatibility with IBC and ABCI2. # The dependences among tm2, gnovm and gno.land - gno.land depends on tm2 and gnovm - gnovm depends on tm2 - tm2 is independent to gnovm and gno.land ```mermaid flowchart LR A[gno.land]--> B[gnovm] --> C[tm2] A --> C ``` # Discussion ## client commands We aim to establish a clear dependency structure between these components as outlined earlier. Ideally we'd like to have clear dependency between these components as listed above At present, tm2/pkg/crypto/keys/client/root.go is dependent on gno.land/pkg/sdk/vm. We're considering moving the following commands to gno.land/cmd/gnokey/ and eliminating the newMakeTxCmd() from cmd.AddSubCommands() in tm2/pkg/crypto/client/root.go: - addpkg.go - call.go - send.go - maketx.go However, these files rely on the private config structure and private methods defined in tm2/pkg/crypto/keys/client/. Until we decide the next steps, these files will remain in tm2, which means that tm2/ continues to depend on gno.lang/ for now. ## genproto As for the genproto, we need to decide where misc/genproto/genproto.go should be placed. ## sample tendermint2 program Since we've moved out applications that depend on gno in tm2, we need to consider whether we should reintroduce the sample tendermint application or create a new one. [tendermint classic main.go](https://github.com/tendermint/classic/tree/master/cmd/tendermint) </p> </details> <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> --------- Co-authored-by: piux2 <> Co-authored-by: Manfred Touron <[email protected]>
Description
Part of #692 #585
BREAKING CHANGE: In GitHub Action Flow, the build script needs to be updated on moving tm2txsync from tm2/cmd to gno.land/cmd
gno.land/
It provides information and tools necessary for running gno.land node. It includes
In the future, we plan to consolidate tools for node deployment and monitoring. The future plan also includes supporting both IBC and ICS.
gnovm/
Gnovm is agnostic to any SDK or chain.
It is intended for gno contract developers and gno VM developers. It includes
Future plans include making gnovm compatible with IBC
tm2/
Tendermint2 is a BFT consensus engine and a set of tools for blockchain builders
More details can be found at
https://github.com/tendermint/tendermint2#readme
In the future, tm2 plans to:
The dependences among tm2, gnovm and gno.land
Discussion
client commands
We aim to establish a clear dependency structure between these components as outlined earlier.
Ideally we'd like to have clear dependency between these components as listed above
At present, tm2/pkg/crypto/keys/client/root.go is dependent on gno.land/pkg/sdk/vm.
We're considering moving the following commands to gno.land/cmd/gnokey/ and eliminating the newMakeTxCmd() from cmd.AddSubCommands() in tm2/pkg/crypto/client/root.go:
However, these files rely on the private config structure and private methods defined in tm2/pkg/crypto/keys/client/. Until we decide the next steps, these files will remain in tm2, which means that tm2/ continues to depend on gno.lang/ for now.
genproto
As for the genproto, we need to decide where misc/genproto/genproto.go should be placed.
sample tendermint2 program
Since we've moved out applications that depend on gno in tm2, we need to consider whether we should reintroduce the sample tendermint application or create a new one.
tendermint classic main.go
Contributors Checklist
BREAKING CHANGE: xxx
message was included in the descriptionMaintainers Checklist
CONTRIBUTING.md
BREAKING CHANGE:
in the body)