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

Making the std package API more user-friendly #1576

Open
leohhhn opened this issue Jan 23, 2024 · 2 comments
Open

Making the std package API more user-friendly #1576

leohhhn opened this issue Jan 23, 2024 · 2 comments
Assignees
Labels
breaking change Functionality that contains breaking changes

Comments

@leohhhn
Copy link
Contributor

leohhhn commented Jan 23, 2024

Description

This issue is meant to discuss potential changes to the std package API & naming. There are multiple issues/discussions on how this should happen, ie #1475 & #1521. Extracting parts of @thehowl's proposition on renaming things out of #1475 for std:

  • DerivePkgAddr > AddressFromPath
  • GetCallerAt > CallerAt
  • GetBanker(BankerType) >NewBanker(BankerType)
  • GetOrigSend > OriginSend
  • CurrentRealm, PreviousRealm (instead of PrevRealm) should return the current Realm object and the previous caller (be it another realm or an EOA). This also needs to be discussed as these functions can be called in packages, and that isn't really consistent or logical naming.
  • CurrentRealmPath > just use CurrentRealm.Path
  • GetChainID > ChainID
  • GetHeight > BlockHeight
  • GetOrigCaller >OriginCaller
  • GetOrigPkgAddr - the naming of this function is unintuitive - we are using the terminology "Origin Caller" to get the EOA that signed the initial transaction, but we expect the Origin Package to be the current realm/package that is having its code executed. IMHO, this should be completely removed in favor of CurrentRealm.

A broader discussion is related to if we are going to keep IsOriginCall & AssertOriginCall. These functions basically limit the caller of the transaction to be an EOA - and this blocks a lot of possible functionality, like account abstraction, smart contract wallets, & calling realms code through your code. I am personally in favor of simply removing these from the codebase.

While we are on this note, consider renaming PkgPath > Path - it is confusing that we can have packages that have package paths, but we can also have realms that have package paths ("you need to get the realm package path"). This would include making the changes in all tools as well. With this, we can also discuss renaming addpkg commands in gnokey & related tools to deploy/upload/publish.

I am not discussing the std package split as per @thehowl's comment, since AFAIK it is blocked by some other issues.

Another thing to keep in mind with these discussions is to have the std.TestXXX functions match the above, as discussed in #1521.

Please, read the other issues and put down your thoughts - this is a relatively urgent issue, since if we want to do any big renaming efforts, it would be better before we have a large dev base with a lot of apps, because the renaming would be a breaking change.

@moul
Copy link
Member

moul commented Jan 23, 2024

DerivePkgAddr > AddressFromPath

"Address" and "Path" have ambiguous meanings.

Perhaps we could consider implementing something like this in Go:

type Package struct {
    Path string
}

func (p Package) Addr() std.Address {}

CurrentRealm, PreviousRealm (instead of PrevRealm)...

I agree to add CurrentRealm, but I prefer using PrevRealm instead of PreviousRealm. I'm open to discussing this further.


Everything else appears fine at the moment.

@moul
Copy link
Member

moul commented Jan 24, 2024

Related with #1024

@thehowl thehowl added this to the 🏗4️⃣ test4.gno.land milestone Apr 17, 2024
@thehowl thehowl self-assigned this Apr 17, 2024
@Kouteki Kouteki added the breaking change Functionality that contains breaking changes label Apr 19, 2024
thehowl added a commit that referenced this issue Jun 19, 2024
Merge order:

1. #1700 
2. #1702
3. #1695 (this one!) -- review earlier ones first, if they're still
open!

This PR modifies the Gno transpiler (fka precompiler) to use Gno's
standard libraries rather than Go's when performing transpilation. This
creates the necessity to transpile Gno standard libraries, and as such
support their native bindings. And it removes the necessity for a
package like `stdshim`, and a mechanism like `stdlibWhitelist`.

- Fixes #668. Fixes #1865.
- Resolves #892.
- Part of #814. 
- Makes #1475 / #1576 possible without using hacks like `stdshim`.

cc/ @leohhhn @tbruyelle, as this relates to your work

## Why?

- This PR enables us to perform Go type-checking across the board, and
not use Go's standard libraries in transpiled code. This enables us to
_properly support our own standard libraries_, such as `std` but any
others we might want or need.
- It also paves the way further to go full circle, and have Gno code be
transpiled to Go, and then have "compilable" gno code

## Summary of changes

- The transpiler has been thoroughly refactored.
- The biggest change is described above: instead of maintaing the import
paths like `"strconv"` and `"math"` the same (so using Gno's stdlibs in
Gno, and Go's in Go), the import paths for standard libraries is now
also updated to point to the Gno standard libraries.
- Native functions are handled by removing their definitions when
transpiling, and changing their call expressions where appropriate. This
links the transpiled code directly to their native counterparts.
  - This removes the necessity for `stdlibWhitelist`. 
- As a consequence, `stdshim` is no longer needed and has been removed.
- Test files are still not "strictly checked": they may reference
stdlibs with no matching source, and will not be tested when running
with `--gobuild`. This is because packages like `fmt` have no
representation in Gno code; they only exist as injections in
`tests/imports.go`. I'll fix this eventually :)
- The CLI (`gno transpile`) has been changed to reflect the above
changes.
- Flag `--skip-fmt` has been removed (the result of transpile is always
formatted, anyway), and `--gofmt-binary` too, obviously. `gno transpile`
does not perform validation, but will gladly provide helpful validation
with the `--gobuild` flag.
- There is another PR that adds type checking in `gno lint`, without
needing to run through the transpilation step first:
#1730
- It now works by default by looking at "packages" rather than
individual files. This is necessary so that when performing `transpile`
on the `examples` directory, we can skip those where the gno.mod marks
the module as draft. These modules make use of packages like "fmt",
which because they don't have an underlying gno/go source, cannot be
transpiled.
- Running with `-gobuild` now handles more errors correctly; ie., all
errors not previously captured by the `errorRe` which only matches those
pertaining to a specific file/line.
  - `gnoFilesFromArgs` was unused and as such deleted
- `gnomod`'s behaviour was slightly changed.
- I am of the opinion that `gno mod download` should not precompile what
it downloads; _especially_ to gather the dependencies it has. I've
changed it so that it does a `OnlyImports` parse of the file it
downloads to fetch additional dependencies

Misc:

- `Makefile` now contains a recipe to calculate the coverage for
`gnovm/cmd/gno`, and also view it via the HTML interface. This is needed
as it has a few extra steps (which @gfanton already previously added in
the CI).
- Realms r/demo/art/gnoface and r/x/manfred_outfmt have been marked as
draft, as they depend on packages which are not actually present in the
Gno standard libraries.
  - The transpiler now ignores draft packages by default.
- `ReadMemPackage` now also considers Go files. This is meant to have
on-chain the code for standard libraries like `std` which have native
bindings. We still exclude Go code if it's not in a standard library.
- `//go:build` constraints have been removed from standard libraries, as
go files can only have one and we already add our own when transpiling

## Further improvements

after this PR

- Scope understanding in `transpiler` (so call expressions are not
incorrectly rewritten)
- Correctly transpile gno.mod

---------

Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
gfanton pushed a commit to gfanton/gno that referenced this issue Jul 23, 2024
Merge order:

1. gnolang#1700 
2. gnolang#1702
3. gnolang#1695 (this one!) -- review earlier ones first, if they're still
open!

This PR modifies the Gno transpiler (fka precompiler) to use Gno's
standard libraries rather than Go's when performing transpilation. This
creates the necessity to transpile Gno standard libraries, and as such
support their native bindings. And it removes the necessity for a
package like `stdshim`, and a mechanism like `stdlibWhitelist`.

- Fixes gnolang#668. Fixes gnolang#1865.
- Resolves gnolang#892.
- Part of gnolang#814. 
- Makes gnolang#1475 / gnolang#1576 possible without using hacks like `stdshim`.

cc/ @leohhhn @tbruyelle, as this relates to your work

## Why?

- This PR enables us to perform Go type-checking across the board, and
not use Go's standard libraries in transpiled code. This enables us to
_properly support our own standard libraries_, such as `std` but any
others we might want or need.
- It also paves the way further to go full circle, and have Gno code be
transpiled to Go, and then have "compilable" gno code

## Summary of changes

- The transpiler has been thoroughly refactored.
- The biggest change is described above: instead of maintaing the import
paths like `"strconv"` and `"math"` the same (so using Gno's stdlibs in
Gno, and Go's in Go), the import paths for standard libraries is now
also updated to point to the Gno standard libraries.
- Native functions are handled by removing their definitions when
transpiling, and changing their call expressions where appropriate. This
links the transpiled code directly to their native counterparts.
  - This removes the necessity for `stdlibWhitelist`. 
- As a consequence, `stdshim` is no longer needed and has been removed.
- Test files are still not "strictly checked": they may reference
stdlibs with no matching source, and will not be tested when running
with `--gobuild`. This is because packages like `fmt` have no
representation in Gno code; they only exist as injections in
`tests/imports.go`. I'll fix this eventually :)
- The CLI (`gno transpile`) has been changed to reflect the above
changes.
- Flag `--skip-fmt` has been removed (the result of transpile is always
formatted, anyway), and `--gofmt-binary` too, obviously. `gno transpile`
does not perform validation, but will gladly provide helpful validation
with the `--gobuild` flag.
- There is another PR that adds type checking in `gno lint`, without
needing to run through the transpilation step first:
gnolang#1730
- It now works by default by looking at "packages" rather than
individual files. This is necessary so that when performing `transpile`
on the `examples` directory, we can skip those where the gno.mod marks
the module as draft. These modules make use of packages like "fmt",
which because they don't have an underlying gno/go source, cannot be
transpiled.
- Running with `-gobuild` now handles more errors correctly; ie., all
errors not previously captured by the `errorRe` which only matches those
pertaining to a specific file/line.
  - `gnoFilesFromArgs` was unused and as such deleted
- `gnomod`'s behaviour was slightly changed.
- I am of the opinion that `gno mod download` should not precompile what
it downloads; _especially_ to gather the dependencies it has. I've
changed it so that it does a `OnlyImports` parse of the file it
downloads to fetch additional dependencies

Misc:

- `Makefile` now contains a recipe to calculate the coverage for
`gnovm/cmd/gno`, and also view it via the HTML interface. This is needed
as it has a few extra steps (which @gfanton already previously added in
the CI).
- Realms r/demo/art/gnoface and r/x/manfred_outfmt have been marked as
draft, as they depend on packages which are not actually present in the
Gno standard libraries.
  - The transpiler now ignores draft packages by default.
- `ReadMemPackage` now also considers Go files. This is meant to have
on-chain the code for standard libraries like `std` which have native
bindings. We still exclude Go code if it's not in a standard library.
- `//go:build` constraints have been removed from standard libraries, as
go files can only have one and we already add our own when transpiling

## Further improvements

after this PR

- Scope understanding in `transpiler` (so call expressions are not
incorrectly rewritten)
- Correctly transpile gno.mod

---------

Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Miloš Živković <milos.zivkovic@tendermint.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Functionality that contains breaking changes
Projects
Status: No status
Development

No branches or pull requests

4 participants