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

chore: relocate tm2/std.MemFile in gnovm/std #2910

Merged
merged 15 commits into from
Oct 25, 2024

Conversation

moul
Copy link
Member

@moul moul commented Oct 5, 2024

This PR removes "gno.land" from all tm2/.../*.go files.

@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/mv memfile chore: relocate tm2/std.MemFile in gnovm/std Oct 5, 2024
Copy link

codecov bot commented Oct 5, 2024

Codecov Report

Attention: Patch coverage is 86.11111% with 5 lines in your changes missing coverage. Please review.

Project coverage is 63.59%. Comparing base (a478348) to head (9104fe6).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
gno.land/pkg/keyscli/run.go 66.66% 1 Missing ⚠️
gno.land/pkg/sdk/vm/keeper.go 0.00% 1 Missing ⚠️
gnovm/pkg/gnolang/machine.go 75.00% 1 Missing ⚠️
gnovm/pkg/gnolang/nodes.go 83.33% 1 Missing ⚠️
gnovm/pkg/gnolang/store.go 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2910      +/-   ##
==========================================
+ Coverage   63.39%   63.59%   +0.19%     
==========================================
  Files         565      566       +1     
  Lines       79457    79614     +157     
==========================================
+ Hits        50375    50633     +258     
+ Misses      25691    25589     -102     
- Partials     3391     3392       +1     
Flag Coverage Δ
contribs/gnodev 60.57% <ø> (ø)
contribs/gnofaucet 14.82% <ø> (-0.95%) ⬇️
gno.land 67.37% <77.77%> (ø)
gnovm 67.87% <87.50%> (+<0.01%) ⬆️
misc/genstd 79.72% <ø> (ø)
tm2 62.41% <100.00%> (-0.05%) ⬇️

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
Copy link
Member Author

moul commented Oct 5, 2024

I chose to create gnovm/pkg/std, but we should consider merging it with gnovm/pkg/stdlibs/std. What do you think?

I removed the .pb.go files that were added forcefully. Should I revert this change?

Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
@thehowl
Copy link
Member

thehowl commented Oct 5, 2024

I have a more general question - why do we name anything std? Is there any standard involved?

@moul moul marked this pull request as ready for review October 5, 2024 10:05
@moul
Copy link
Member Author

moul commented Oct 5, 2024

I have a more general question - why do we name anything std? Is there any standard involved?

Right now, we refer to these as "lowest level types." The closest name we have is "sys," and the system contracts are currently named "r/sys." We also considered naming them "r/std." Essentially, this serves as our standard for low-level system components.

In this repository, we have three standard packages designed to be imported to connect with the components without loading other parts (lightweight, no side effects).

An alternative for the Go side would be to rename them to "types." However, it's important to distinguish between "system/low-level types" and "all types." One invites connection with a minimal API (communication layer), while the other is more internal and advanced.

I'm open to reconsidering a renaming, although I believe we will likely prefer to stick with the current approach. My question is about balancing independence with unifying gnovm's standard messages.

Signed-off-by: moul <[email protected]>
@moul
Copy link
Member Author

moul commented Oct 5, 2024

By the way, the relocation is complete, and the CI is green. I have two other PRs in mind that will depend on merging this one. Please feel free to review it, and we can discuss the "naming" topic later, maybe.

@moul moul mentioned this pull request Oct 5, 2024
4 tasks
@moul
Copy link
Member Author

moul commented Oct 8, 2024

Discussing the naming with @gfanton.

Another suitable location for MemFile and MemPackage could be directly in the gnovm/pkg/gnolang package, as they are frequently used by gnovm. However, since we are considering splitting this package, the question arises: "If the type was initially implemented in gnovm, what would be a good location if we split it?"

gno.land/pkg/sdk/vm/msgs.go Outdated Show resolved Hide resolved
gno.land/pkg/sdk/vm/msgs.go Outdated Show resolved Hide resolved
@zivkovicmilos
Copy link
Member

Let me take a peek at this before merging please 🙏

@gfanton
Copy link
Member

gfanton commented Oct 10, 2024

I also feel that std can be confusing due to its use everywhere.

@moul, what do you think about simply prefixing the package name (not the structure, as mentioned in the review meeting) like std to stdsys (or std-sys), for example? Even if it seems redundant in the package path, it could enhance file and import readability.

In any case, I don't believe we should block this PR for this, as it's not a regression. Let's open an issue so we can revisit this later.

@moul
Copy link
Member Author

moul commented Oct 25, 2024

TODO: relocate to gnovm/types.go.
Edit: done.

@thehowl thehowl merged commit 603f6d3 into gnolang:master Oct 25, 2024
118 checks passed
jefft0 added a commit to gnolang/gnonative that referenced this pull request Oct 30, 2024
PR gnolang/gno#2910 moved `MemPackage` to the
gnovm package. We want to track the latest gnolang/gno code. This PR
updates to use the latest gnolang/gno and changes the reference of
`MemPackage` to gnovm. After we merge this PR, we will update
gnokey-mobile to use this latest gnonative, which is possible because
gnokey-mobile no longer uses a replace for gnolang/gno.

Signed-off-by: Jeff Thompson <[email protected]>
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: Done
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants