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: move standard library initialization to genesis transactions #3168

Draft
wants to merge 118 commits into
base: master
Choose a base branch
from

Conversation

n0izn0iz
Copy link
Contributor

@n0izn0iz n0izn0iz commented Nov 20, 2024

WIP opening for discussions

Closes #2730

Depends on #3119

TODO:

  • gnoland start -lazy
  • gnodev
  • test envs
  • decide if we want to reuse MsgAddPackage or introduce MsgAddStdlib (see this comment) -> Going for keeping MsgAddPackage for now
  • gno fmt
  • gno doc
  • gnogenesis
  • optimize (currently gnoland test take more than 30 min)
  • optimize 2 (currently gnovm test take 20min)
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

@n0izn0iz n0izn0iz mentioned this pull request Nov 26, 2024
6 tasks
Signed-off-by: Norman Meier <[email protected]>
@Kouteki Kouteki added the in focus Core team is prioritizing this work label Nov 26, 2024
@@ -33,9 +33,8 @@ jobs:
uses: ./.github/workflows/test_template.yml
with:
modulepath: ${{ inputs.modulepath }}
tests-timeout: "30m"
tests-timeout: "60m"
Copy link
Member

Choose a reason for hiding this comment

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

I think we discussed why this was needed privately, but it's worth nothing here as well 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to make an optimization in this PR to keep gnoland test around 20min like on master
I keep you posted

@@ -92,6 +93,9 @@ type Node struct {
var DefaultFee = std.NewFee(50000, std.MustParseCoin(ugnot.ValueString(1000000)))

func NewDevNode(ctx context.Context, cfg *NodeConfig) (*Node, error) {
stdlibsDeployer := crypto.MustAddressFromString("g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5") // test1, FIXME: replace
Copy link
Member

Choose a reason for hiding this comment

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

This is fixable I think with @Villaquiranm's PR

Copy link
Contributor Author

@n0izn0iz n0izn0iz Nov 26, 2024

Choose a reason for hiding this comment

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

Yeah I think I'll wait on it
EDIT: actually I should probably use the same address as other packages, I will address this before merge

examples/gno.land/r/demo/banktest/z_2_filetest.gno Outdated Show resolved Hide resolved
@@ -116,7 +116,7 @@ func TestStart_Lazy(t *testing.T) {
io.SetErr(commands.WriteNopCloser(mockErr))

// Create and run the command
ctx, cancelFn := context.WithTimeout(context.Background(), 10*time.Second)
ctx, cancelFn := context.WithTimeout(context.Background(), 20*time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

I organically hate this test :)

Copy link
Member

Choose a reason for hiding this comment

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

It's a constant reminder of the insane tech debt we have, that's preventing us from easily spinning up configurable clusters in unit tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as it's organic 🥬 ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to restore the master value on this with opti

gno.land/pkg/gnoland/genesis.go Outdated Show resolved Hide resolved
gnovm/cmd/gno/doc.go Show resolved Hide resolved
gnovm/stdlibs/std/context.go Outdated Show resolved Hide resolved
gnovm/tests/stdlibs/std/std.gno Outdated Show resolved Hide resolved
gnovm/tests/stdlibs/std/std.go Outdated Show resolved Hide resolved
gnovm/memfile.go Outdated
reFileName = regexp.MustCompile(`^([a-zA-Z0-9_]*\.[a-z0-9_\.]*|LICENSE|README)$`)
rePkgName = regexp.MustCompile(`^[a-z][a-z0-9_]*$`)
rePkgOrRlmPath = regexp.MustCompile(`^gno\.land\/(?:p|r)(?:\/_?[a-z]+[a-z0-9_]*)+$`)
rePkgOrRlmOrStdlibPath = regexp.MustCompile(`^gno\.land\/(?:p|r)(?:\/_?[a-z]+[a-z0-9_]*)+|(?:[a-z]+[a-z0-9_]*)(?:\/_?[a-z]+[a-z0-9_]*)*$`)
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about breaking up this regex?

@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 1, 2024

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: n0izn0iz/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

@zivkovicmilos
Copy link
Member

@n0izn0iz
What's the status of this PR?

@n0izn0iz
Copy link
Contributor Author

n0izn0iz commented Dec 3, 2024

It passes gno.land and gnovm tests but it increases gno.land test time to 40+ minutes so I need to find a way to optimize packages loading

There has been a recent change in gnodev that require an update on my side, I still need to address that

I would love some feedback on the direction it's taking

@Kouteki
Copy link
Contributor

Kouteki commented Jan 13, 2025

I would love some feedback on the direction it's taking

Summoning @thehowl :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in focus Core team is prioritizing this work 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Move standard library initialization to genesis transactions
5 participants