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

feat: support custom VM domain #2911

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

feat: support custom VM domain #2911

wants to merge 36 commits into from

Conversation

moul
Copy link
Member

@moul moul commented Oct 5, 2024

Introducing the concept of "ChainDomain," a local primary domain for packages.

The next step, which will be another PR, is to ensure that we can launch a gnoland/gnodev instance while importing a local folder or using a genesis to preload packages from other domains. This will allow users to add packages only to the primary domain while accessing packages from multiple domains. The result will be a preview of the upcoming IBC era, where a single chain can add packages only to its domain but can fetch missing dependencies from other registered zones.

  • gnovm unaware of gno.land, just accepting valid domains
  • vmkeeper initialized with a domain
  • Stdlib to know the current primary domain + new std.ChainDomain
  • new unit tests around custom domains

Depends on #2910
Depends on #3003
Blocks a new PR that will add multidomain support.
Related with #2904 (comment)

@moul moul self-assigned this Oct 5, 2024
@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 Oct 5, 2024
@moul moul changed the title dev/moul/gno domain feat: support custom VM domain Oct 5, 2024
Copy link

codecov bot commented Oct 5, 2024

Codecov Report

Attention: Patch coverage is 72.34043% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gno.land/pkg/gnoland/node_inmemory.go 0.00% 8 Missing ⚠️
gno.land/pkg/sdk/vm/keeper.go 80.00% 2 Missing and 1 partial ⚠️
gnovm/stdlibs/std/native.go 0.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
gno.land/cmd/gnoland/start.go Outdated Show resolved Hide resolved
@moul moul marked this pull request as ready for review October 6, 2024 09:27
@moul moul requested a review from thehowl as a code owner October 6, 2024 09:27
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
@moul moul marked this pull request as ready for review November 15, 2024 19:56
@gh-gno-bot
Copy link

gh-gno-bot commented Nov 28, 2024

Merge Requirements

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🔴 Maintainers must be able to edit this pull request
🟢 The pull request head branch must be up-to-date with its base

Details
Maintainers must be able to edit this pull request

If

🟢 Condition met
└── 🟢 On every pull request

Then

🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request

The pull request head branch must be up-to-date with its base

If

🟢 Condition met
└── 🟢 On every pull request

Then

🟢 Requirement satisfied
└── 🟢 Head branch (dev/moul/gno-domain) is up to date with base (master): behind by 0 / ahead by 36

Manual Checks

  • The pull request description provides enough details
Details
The pull request description provides enough details

If

🟢 Condition met
└── 🟢 Not (🔴 Pull request author is a member of the team: core-contributors)

Can be checked by

  • team core-contributors

Copy link
Member

@MikaelVallenet MikaelVallenet left a comment

Choose a reason for hiding this comment

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

i also found that it does not work with gnodev for now, the params should be added to genesis state in contribs/gnodev/pkg/dev/node.go and added into every execContext in gno.land/pkg/sdk/vm/keeper.go (it's what i did on my side with chain tz to make it work with gnodev, maybe it's not the good way)

gno.land/pkg/gnoland/node_inmemory.go Show resolved Hide resolved
gno.land/genesis/genesis_params.toml Show resolved Hide resolved
@moul
Copy link
Member Author

moul commented Nov 28, 2024

i also found that it does not work with gnodev for now, the params should be added to genesis state in contribs/gnodev/pkg/dev/node.go and added into every execContext in gno.land/pkg/sdk/vm/keeper.go (it's what i did on my side with chain tz to make it work with gnodev, maybe it's not the good way)

Do you have an example?

@MikaelVallenet
Copy link
Member

i also found that it does not work with gnodev for now, the params should be added to genesis state in contribs/gnodev/pkg/dev/node.go and added into every execContext in gno.land/pkg/sdk/vm/keeper.go (it's what i did on my side with chain tz to make it work with gnodev, maybe it's not the good way)

Do you have an example?

So here is the addition of params into genesis: c3ecc97
Here is the file where i add chain tz to every ExecContext: https://github.com/gnolang/gno/pull/3193/files#diff-3663e30f0c0e01d8d47bb7974d605b4cf12d1a244970c6a9bbba55fb0be6aced
Without this, running gnodev --<param>=<value> will not work, (not sure it's good to do the way i did tbh)

@moul
Copy link
Member Author

moul commented Nov 28, 2024

So here is the addition of params into genesis: c3ecc97

I suggest you switch to my method. I implemented this in gnoland, which is used by pkg/integration. You made a similar change, but only in gnodev, making this feature harder to use and test outside of gnodev.

Here is the file where i add chain tz to every ExecContext: #3193 (files)

If I'm not mistaken, that's what I did as well. Do you see any places where I overlooked it?

@MikaelVallenet
Copy link
Member

So here is the addition of params into genesis: c3ecc97

I suggest you switch to my method. I implemented this in gnoland, which is used by pkg/integration. You made a similar change, but only in gnodev, making this feature harder to use and test outside of gnodev.

Here is the file where i add chain tz to every ExecContext: #3193 (files)

If I'm not mistaken, that's what I did as well. Do you see any places where I overlooked it?

  • Yes this one is just the commit about gnodev, but i think the change should be applied to both places

  • I can see on your PR
    you did not add ChainDomain in ExecContext return value in QueryEval and QueryEvalString and Call function maybe AddPackage function too (github diff is a bit broken and does not display func name). I added it to these places, but i dont know exactly what QueryEval or QueryEval are used for, just felt it could fix my gnodev issue since it returns ExecContext and it works

@moul
Copy link
Member Author

moul commented Nov 28, 2024

Yes this one is just the commit about gnodev, but i think the change should be applied to both places

`gnodev` -> `gno.land/pkg/integration` > `gno.land/pkg/gnold`
    CLI       ->             helper with new arg  >        actual place where we create a dynamic genesis

The param override should occur in gnoland. gnodev should simply configure its CLI parser and pass the new option, but it shouldn't manipulate the genesis directly (not for this case).

I can see on your PR
you did not add ChainDomain in ExecContext return value in QueryEval and QueryEvalString and Call function maybe AddPackage function too (github diff is a bit broken and does not display func name). I added it to these places, but i dont know exactly what QueryEval or QueryEval are used for, just felt it could fix my gnodev issue since it returns ExecContext and it works

I should definitely add them. Thank you.

  • TODO

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 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: In Progress
Status: 🎯 Current Topics
Status: In Review
Development

Successfully merging this pull request may close these issues.

6 participants