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: std re-organization #2425

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Jun 24, 2024

This is a work-in-progress PR, but it contains some key ideas of the new design

References:

This will be worked into a complete state so that we can see the full extent of the transition to the new system, then it will be broken down into a few PRs for ease of reviewing, similar to what happened with #1695.

Objectives of this effort:

  • Remove std in favour of chain, runtime and debug/testing.
  • Remove testing stdlibs.
    (This has not been discussed much yet, but the current "dual system" for standard libraries, is that p/ packages that serve as "testing aids" cannot use test-context functions due to the type checking. So I'm unifying the stdlibs, but disabling test-only features on-chain).
  • standardise import path instead of pkgpath
    This is something I wanted to add for a while for consistency with Go.
  • make sure internal/ rules are respected
    Ie. make them work as in Go.
  • Change "send" into "deposit"

These are all individually up for discussion, but I think they fit nicely into a better std; so here it is.

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.

@grepsuzette
Copy link
Contributor

@thehowl A bit late to the party, but it's still a draft. As said in #1475, I advocate renaming std to gno.
There's too many benefits for beginners to not discuss it, /me think.

// associated import path.
type Realm struct {
address Address
importPath string
Copy link
Member

@moul moul Jul 12, 2024

Choose a reason for hiding this comment

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

What's the problem with PkgPath?

Edit: I think we should keep pkgPath over importPath. Because ultimately, the package path is an identifier which, yes, can be imported, but it's an identifier first, just as a std.Address is an identifier for an account. This one is the identifier for the realm and its assets, and the cool thing is that it's importable. It's like we would say that Go's package path can be imported, so let's not name them ImportPath. But we can also go mod download them, so let's not rename them goModDownloadPath either. I think we should be clear that the identifier is the unique identifier of a package, and then importPath is currently the same 1-1, but maybe in the future, we'll support import aliases for versioning or to support, for instance, r/morgan/foo and r/g1morganxxx/foo to be the same pkgPath but different importPath.

Copy link
Contributor

@leohhhn leohhhn Sep 23, 2024

Choose a reason for hiding this comment

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

I am for a different name that pkgpath, because it conflicts most of the time when explaining the topics, making it very confusing. possibly URL, URI, ID, just path, etc. in the end its a unique identifier for code, and it can be used to look up code or import code.

  • realm path - better than realm package path
  • pure package path - better than pure package package path.

// Package runtime allows access to the GnoVM's runtime information.
package runtime

func NewReadOnlyBanker() chain.Banker {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the benefit of having a distinct runtime package that ultimately implements helpers that will return chain types. Wouldn't it be simpler and less confusing to have a single package that defines the types and implements the helpers?

@moul
Copy link
Member

moul commented Jul 12, 2024

I like the refactoring idea and believe that most of the renames make sense. However:

  • I don't see the need for separate chain and runtime packages instead of a single package.
  • I prefer keeping std or use os more than using chain and/or runtime or gno.
  • I really like the new testing and context approaches. <3
  • My comment about pkgPath vs importPath.

}

func (c Context) Run(f func()) {
}
Copy link
Member

@moul moul Jul 12, 2024

Choose a reason for hiding this comment

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

func CurrentFileLine() string {
	return ufmt.Sprintf("%s:%d", prevFrame.Filepath, prevFrame.LineNumber)
}

Or better GetFrameAt(x int).FileLine().

could be useful for things like uassert.

import (
"chain"
)

Copy link
Member Author

Choose a reason for hiding this comment

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

XXX:

testing calls each test using a new Context{} (so that they're running always on a clean state)

same goes for subtests

a test may still call SetContext to just alter variables

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 should just run a new gnomachine, it’s not only about context,but about sharing any state (global variable too).

func NewSendBanker() chain.Banker {
}

func NewIssueBanker() chain.Banker {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func NewIssueBanker() chain.Banker {
func NewIssuingBanker() chain.Banker {

func AssertOriginCall() {
}

func CallerRealm() chain.Realm {
Copy link
Contributor

@leohhhn leohhhn Sep 23, 2024

Choose a reason for hiding this comment

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

I am of a hot take that we should completely remove the possibility of a realm being an EOA. It's a very confusing concept, especially when talking about realm package, package path, etc. Then, we could just have Caller, which can be a realm or an EOA/user.

func CurrentRealm() chain.Realm {
}

func Deposits() chain.Coins {
Copy link
Contributor

@leohhhn leohhhn Sep 23, 2024

Choose a reason for hiding this comment

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

We need to make a distinction between prevrealm deposits and origin deposits; rather, you only need to read the immediate previous realm deposit; I'm not sure of a usecase where origin deposit could be needed.

Related: #1887

// Context contains execution context variables which are used primarily by the
// [runtime] package to give information about the current GnoVM execution.
type Context struct {
IsOrigin bool
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this bool represent? that the caller is the origin caller?


// NewCodeRealm creates a new realm, representing a code realm with a published
// import path, existing on-chain.
func NewCodeRealm(importPath string) Realm {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: No status
Status: Triage
Development

Successfully merging this pull request may close these issues.

4 participants