Skip to content
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

refactor: tm2 clean up against the latest main branch #1080

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

piux2
Copy link
Contributor

@piux2 piux2 commented Aug 30, 2023

Original text from #825

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

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
flowchart LR
    A[gno.land]--> B[gnovm] --> C[tm2]
    A --> C

Loading

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

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.

@piux2 piux2 requested review from a team, jaekwon and moul as code owners August 30, 2023 04:42
@github-actions github-actions bot added 📦 🤖 gnovm Issues or PRs gnovm related 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Aug 30, 2023
@piux2 piux2 changed the title tm2 clean up against the latest main branch refactor: tm2 clean up against the latest main branch Aug 30, 2023
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comments, I'll be closing #825 and redirecting it to this one for clarity.

My 2c on what to do for pkg/crypto/keys/client:

  • move HomeDir to somewhere under gnovm - pkg/util? pkg/fsutil? pkg/cliutil?
    (I like pkg/fsutil)
  • make all the newXCmd functions exported, and all xCfg.
    Rationale: we want to separate addpkg and call from the rest, making client a generic package to create tendermint 2 clients. This allows us to enable that.
  • Make SignHandler unexported (unclear to me why it's exported in the first place -- it's a bit useless as well because signCfg is unexported).
  • Move addpkg and call to either gnovm or gno.land.

If you think this is worth discussing more before deciding, we can go ahead with this one as manfred said in #825 and then tackle addpkg/call in another PR.

tm2/Makefile Show resolved Hide resolved
@thehowl thehowl mentioned this pull request Aug 30, 2023
9 tasks
tm2/Makefile Show resolved Hide resolved
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 💯

I've left a minor comment just for discussion, since the dependency stuck out

tm2/pkg/crypto/keys/client/addpkg.go Show resolved Hide resolved
@moul
Copy link
Member

moul commented Aug 31, 2023

High-level strategy regarding the splits:

  • tm2 should never depend on gno.
  • gnovm can depend on tm2 even if it would be better to not depend on it at all.
  • gno.land depends on both, it's also OK to keep gno.land as a temporary buffer for everything not generic enough with the goal to split later.

Target

  • tm2/*: allow creating tm2 blockchains and CLIs, without gnovm (all the verbs in tm2/vm/sdk, except call and addpkg)
  • gnovm: allow creating tm2 blockchains with gnovm (can depend on tm2); in a far future, we can make it completely independent
  • gno.land: compose tm2 + gnovm & configure settings

Potential intermediary state (not perfect, bug good enough, and flexible)

  • tm2: things that are obviously already generic enough for tm2/ in the target
  • gnovm: only the language, but nothing regarding "tm2 module" yet
  • gno.land: everything mixing gnovm and gno.land, with the goal to extract a few things to gnovm later

gnovm/Makefile Outdated Show resolved Hide resolved
@moul
Copy link
Member

moul commented Aug 31, 2023

  • move HomeDir to somewhere under gnovm - pkg/util? pkg/fsutil? pkg/cliutil?
    (I like pkg/fsutil)

Should stay in tm2, but it makes sense to make this more independent, especially if we start having multiple OS support.
tm2/pkg/fsutil, with a way to pass a "name" parameter in the constructor to make a different homedir for gno.land and foobar.land.

  • make all the newXCmd functions exported, and all xCfg.
    Rationale: we want to separate addpkg and call from the rest, making client a generic package to create tendermint 2 clients. This allows us to enable that.

Yep, another related work is in progress regarding this package in #1047; the strategy was the same 👍

  • Make SignHandler unexported (unclear to me why it's exported in the first place -- it's a bit useless as well because signCfg is unexported).

Yep

  • Move addpkg and call to either gnovm or gno.land.

To gno.land for now; gnovm stays mostly language and tool oriented for now; and later, we'll extract the generic part of gno.land to gnovm/pkg/tm2mod or something like that, to ease creation of tm2 blockchains with gnovm.

@github-actions github-actions bot removed the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 31, 2023
@moul moul merged commit 64f0fd0 into gnolang:master Aug 31, 2023
60 of 61 checks passed
@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants