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

META - splitting and refactoring std #1475

Open
leohhhn opened this issue Dec 20, 2023 · 18 comments
Open

META - splitting and refactoring std #1475

leohhhn opened this issue Dec 20, 2023 · 18 comments
Assignees

Comments

@leohhhn
Copy link
Contributor

leohhhn commented Dec 20, 2023

This is a META issue centralizing the current state of proposals on how to re-organize the std standard library.

List of proposals

Collected by @thehowl, with reasons and statuses attempted to be summarized by him and possibly incorrect, but all to the best of his knowledge / research.

Split into different packages

Package name ideas (see related gist

  • chain - containing critical chain types, like Address, AddressList.
  • chain/runtime - containing meta-VM functions, like CurrentRealm, PrevRealm, AssertOriginCall.
    • not runtime as it has a different API and usage than Go's runtime
  • chain/banker - containing the Banker and Coin types, and related functions and methods to deal with the native banker.
  • chain/params - containing the SetParamBool, SetParamString, ... types

The division into packages should help to reduce the scope of each package, helping us create modular packages. It's not definitive; some questions remain on where to put the Emit functions.

Why: at the time being, all Gno-specific functions are lumped together in a single, big std package. Naming packages like base, util or common is considered bad practice in Go, where instead packages should be more specific and be concerned with specific purposes.

Status: consensus.

References:

Removing Get prefixes from the functions

  • CallerAt instead of GetCallerAt
  • OrigSend instead of GetOrigSend
  • OriginCaller instead of GetOriginCaller

Why: Effective Go states that "there's nothing wrong with providing getters and setters yourself, and it's often appropriate to do so, but it's neither idiomatic nor necessary to put Get into the getter's name."

Status: consensus.

References:

Expanding abbreviations and using consistent naming

  • PreviousRealm instead of PrevRealm
  • Address instead of Addr
  • Origin instead of Orig

std currently has a variety of places where it uses Addr and others where it uses Address, and other similar abbreviations listed above. The most important thing is to make everything consistent; the proposed way of doing so is by preferring the expanded version.

NOTE: we should keep Pkg as it is instead of Package, as PkgPath is used in many other places and is already established as a "conventional abbreviation".

Why: using consistent naming allows us to avoid becoming the next PHP.

Status: consensus.
References:

Remove std.IsOriginCall

Why: IsOriginCall, is a function that should likely not be used and available to end-users. It only serves to check that the function calling it is called directly by maketx call; and so usages should likely simply converge on AssertOriginCall.

  • AssertOriginCall provides a good pattern to ensure functions are only called directly by end-users.
  • Similarly, std.IsOriginCall should likely be replaced by std.PrevRealm().IsUser(), as PrevRealm ignores frames within the same realm.

NOTE: a previous version considered also the removal of GetOriginCall, in light of discussions (see-also this comment) we're not removing it for now.

Status: consensus (w/out jae).

References:

Adding crypto/bech32

and using it in place of a native DerivePkgAddr.

Why: allows to use pure Gno implementations for DerivePkgAddr, removing the number of natively-defined function. Allows other realms to use and play with bech32 functions as necessary.

other ideas:

  • rename to DeriveRealmAddress? (especially if we only allow realm paths)

Status: consensus (w/out jae).

Work started in #2425, browse code.

Manipulate entire contexts instead of TestSetX functions

Consequently, removing most (all?) Test- functions.

See example testing.Context in #2425

type Context struct {
	IsOrigin     bool
	CurrentRealm chain.Realm
	Deposit      chain.Coins
	ChainID      string
	Height       uint64
	Time         time.Time
	Banker       chain.Banker
	Logger       bool // TODO
}

To be used either with a Run(closure func()) method which executes the function in the given context; OR with a testing.SetContext function now that test functions are executed in separete contexts (#2975).

Why: allows to have a single context, accessible programmatically, to modify the entire testing context and allow constructing other helper functions on top, with behaviours defined in Gno. (Think a SkipHeight which modifies both the height and the timestamp, rather than having this defined in a TestSkipHeights native function).

Status: consensus (w/out jae).

References:

Realm-scoped Deposits instead of OrigSend

Why: we will likely want a mechanism for a realm to send coins to another and access functions/methods guarded by OrigSend; similarly to the move we've done from GetOrigCall to PrevRealm().Addr(). For this reason, a rename to Deposits is hypothesized, which is reset by default when a realm calls another realm (exact mechanism for how a Deposit is set TBD). Similarly, the "Banker changes" below propose to change the OrigSend banker to an Allowance banker, to reflect the shift to Deposit or similar mechanism.

Status: pending discussion.

Note: need, even for the end-user, to prevent cases of MsgRun being able to abuse a OrigSend-guarded function, by calling it repeatedly (ie. the Deposit should possibly change if calling the realm again, at another time.)

ImportPath instead of PkgPath

Why: PkgPath , aside from containing an abbreviation, can be ambiguous (as it refers to the path used by both pure packages and realms), and Go seems to more generally adopt the name "import path".

Status: pending discussions.

References:

Banker changes

func NewReadOnlyBanker() Banker {}
func NewAllowanceBanker(cz Coins) Banker {}
func NewSendBanker() Banker {}
func NewIssuingBanker() Banker {}

type Banker interface {
	// Balance returns the balance in the bank for the given address, as a list
	// of coins.
	Balance(addr Address) (dst Coins)

	// Supply returns the circulating supply of the given denomination.
	Supply(denom string) int64

	// Send sends the given Coins between the from and to addresses.
	Send(from, to Address, amt Coins)

	// Issue issues the coin with the given denomination
	Issue(addr Address, denom string, amount int64)
	Burn(addr Address, denom string, amount int64)
}

This is mostly a refactoring of the API, partly also related to the GRC20 refactor. In summary:

  • Instead of using BankerType, create different constructors for different banker types.
  • Remove OrigSendBanker, removing the need also for a OrigSendSpent, making the API similar to that of grc20 with NewAllowanceBanker.
  • Rename methods into shorter variants / hopefully clearer:
    • Balance instead of GetCoins
    • Supply instead of TotalCoin
    • Send instead of SendCoin
    • Issue instead of IssueCoin. (consider: Mint)
    • Burn instead of RemoveCoin.
  • Additionally, coin denominations are consistently the full coin denomination with the import path, including in Issue and Burn - see: Expect full denoms in Banker.IssueCoin and Banker.RemoveCoin #2107
    • We can consider helper methods like chain.Realm.Denom to construct denominations from package paths.

Why: using constructor allows us to have specific constructors, like for NewAllowanceBanker. Renaming of the methods is done mostly to avoid the redundant Coin. Using full denominations allows the same denom string to be used consistently in all banker methods, rather than having the shorter one for Issue and Burn.

Status: pending discussions.

References:

Removing CallerAt, GetOrigPkgAddr, RawAddress

  • CallerAt seems to be outdated in the current landscape with PrevRealm.
  • GetOrigPkgAddr seems to be outdated with mechanism like AssertOriginCall and PrevRealm.
  • RawAddress seems pointless with a crypto/bech32 package capable of breaking down the bits of the address.

Why: working from a smaller API with only what's proven necessary in real-life usage will allow us to ship a more "conservative" mainnet standard library; and adding new things once we verify a need for them.

Status: pending discussions.

References:

Something other than Realm for Current/PrevRealm

With #667, we agreed to consider end-users as "realms" of their own, considering both end-users and "package" realms to be realms alike; justifying the return of PrevRealm().

We should revisit this, as CurrentRealm / PrevRealm is the only circumstance where we say a realm is ambiguously a end-user or a package with state; in most other usages verbally, on GitHub and in our documentation, a "realm" is simply a package with state.

Status: pending discussions.

References:

@leohhhn leohhhn changed the title Discussion: "std" package naming Discussion: "std" package naming Dec 20, 2023
@waymobetta
Copy link
Contributor

waymobetta commented Dec 20, 2023

I'm with the opinion to avoid overwriting any existing Go stdlib package names to avoid confusing: those who often switch between the languages, the searchability within the codebase (as os is used internally), as well as misc. auto-import tools.

I don't think renaming this library is necessary but I'm curious to hear what others think; or we can determine as a collective rather how to refer to the current std package, as we have been discussing.

@moul
Copy link
Member

moul commented Dec 20, 2023

I have experienced similar feelings on multiple occasions.

Here are my current ideas, listed randomly:

  • Keep the name as std.
  • Change the name to os to align with Golang's package. However, this would result in significant API differences. For a blockchain-less version of gnovm, such as for CLI scripting, we could enable the actual os.
  • Rename it to gno since it is specific to running on a gnovm blockchain. We can imagine a blockchain-less version of a gnovm-powered server with a completely different std package.
  • Move it to a userland package like gno.land/p/std to only have shared std libraries with Go. This would be facilitated by merging the native binding PR (feat(gnovm): improved native bindings #859), but it might create more ambiguity.

@leohhhn
Copy link
Contributor Author

leohhhn commented Dec 20, 2023

@moul

With 1. we will need workarounds in order to discuss/document the std package (not that big of a deal, but still), so essentially I am for modifying the name.

I believe 2. is a totally valid concern. I think "gno" is quite intuitive and unique enough to not get confused with anything.

Are there any technical challenges in regarding to doing this kind of renaming?

@moul
Copy link
Member

moul commented Dec 20, 2023

I can't see any significant technical challenges. It's primarily a challenge related to language design and concerns about adoption and documentation.

Remember that it will be the most widely used import. It's kind of similar with choosing the right keyword, like "return," "panic," "switch," or "for,". It should be something memorable, similar to the well-known if err != nil, which is strongly associated with go.

@deelawn
Copy link
Contributor

deelawn commented Dec 20, 2023

I like the name std and not renaming it to os or something else that overloads an existing go standard library package name. In my opinion, std is the perfect name to avoid naming conflicts with go standard library packages because go does not allow packages to be named std. If we want to apply more meaningful names to groups of functionality within std, then I'd suggest grouping them into subpackages.

I think it is better to keep it out of the normal "userland" package space because of the native bindings that it is built on. I think it makes sense in general for ONLY standard library packages to be able to define native bindings. And in fact, now that the native bindings PR has been merged, it ensures that they are only supported in standard library packages.

So TLDR; I don't support renaming this package but am fine with reorganizing into subpackages.

EDIT: see the strikethrough above; I thought I encountered this the other day but I was wrong

@thehowl
Copy link
Member

thehowl commented Dec 21, 2023

I largely second others in thinking that putting std functionality in os is a tough fit. I think time.Now() returning the block time is a good exception, because as long as we explain the differences, everything else in time still works the same (though, even then, a good consideration might be to rename Now() to something more descriptive like BlockTime(), but I digress).

I don't like std as a name because I think it is too generic and would like for it to be broken down. os is largely not a good fit and a misnomer (you are not accessing the OS, and calling Gno.land an "OS" is a stretch of imagination). But I think runtime, in this regard, is a much better fit. Go's runtime package already exposes many meta-runtime features that make sense within Go programs, so I think in this case we can have the runtime package with a different API.

I propose the following (sorry, I detoured on a redesign of all of the function names, hope you'll forgive me):

  • Package runtime/chain OR just chain (or variants blockchain, bc), containing:
    • types Address, AddressList
    • DerivePkgAddr (better name: AddressFromPkgPath?)
    • EncodeBech32/DecodeBech32
    • type Banker
    • type Coin, Coins
    • type Realm
    • type RawAddress (better: either remove, or make functions like EncodeBech32/DecodeBech32 use it
  • Package runtime, containing:
    • GetCallerAt (better: CallerAt)
    • GetOrigCaller (better: OriginCaller)
    • GetOrigPkgAddr (better: axe this in favour of OriginPkgPath + chain.AddressFromPkgPath)
    • GetBanker (better: NewBanker) + BankerType
    • GetOrigSend (better: OriginSend)
    • CurrentRealm, PrevRealm
    • CurrentRealmPath (better: axe in favour of CurrentRealm.Path)
    • GetChainID (better: ChainID)
    • GetHeight (better: Height, BlockHeight)
    • AssertOriginCall / IsOriginCall

In other words, I like to have a package chain which defines key blockchain-specific types (and utility functions for them), and have a runtime package that accesses the "meta-gnovm" features.

In a kind of way, this is similar to package io (define types, interfaces and useful helper functions) and package os (access the filesystem, often through io types).

@thehowl
Copy link
Member

thehowl commented Dec 21, 2023

Also, one more thought.

I'd reserve gno to be used as a keyword, just-in-case. (And thus not use it as an identifier anywhere)

Maybe the way we implement goroutines is different enough that a better fit is to change the keyword entirely to gno :)

@ilgooz
Copy link
Contributor

ilgooz commented Dec 21, 2023

os/gno or syscall/gno?

Similar to syscall/js

@leohhhn
Copy link
Contributor Author

leohhhn commented Dec 21, 2023

Let this comment be a poll:

Idea 1: as per @thehowl's comment, split std into chain & runtime. Vote with 🚀

Idea 2: simply rename std to gno. Vote with 🎉

Idea 3: keep std as is. Vote with ❤️

In addition, I think that no matter the case, we should decouple classic std functions from functions used for test context editing (ie std.TestSetOrigCaller), and do some name refactoring to make function names more clear from the get-go.

@thehowl thehowl self-assigned this Apr 17, 2024
@thehowl thehowl added this to the 🏗4️⃣ test4.gno.land milestone Apr 17, 2024
@Kouteki Kouteki moved this from Triage to Backlog in 🧙‍♂️gno.land core team Apr 19, 2024
@Kouteki Kouteki moved this from Backlog to Todo in 🧙‍♂️gno.land core team Jun 14, 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>
@thehowl thehowl mentioned this issue Jun 24, 2024
7 tasks
@grepsuzette
Copy link
Contributor

grepsuzette commented Jul 12, 2024

I simply advocate renaming std to gno package.

I do agree splitting packages makes sense as possible solution1, however people are not only going to program exclusively in gno in the future.

Beginners (in gno) and nobody else are the ones who will become long-term users, and for that conversion rate to be maximized I think they need a simple language.

import "gno" is extremely simple, it's as simple and automatic no-brainer as import "std" today. From there, people get autocompletion upon typing gno.|. It doesn't get more simple for beginners.

About reserving gno as a future keyword

The fact go func(){ .. }() exists does not force us to reserve gno func() { .. }().
If we need something special (don't deny, it's a possibility), we can always work a longer name like gnoroutine.

We should choose the most impactful

gno as a keyword is never going to be as impactful as gno as a package.

There not enough material to split?

If I understand correctly, runtime would get just 3 functions: DerivePkgAddr, EncodeBech32, DecodeBech32.

Imagine you go camping with 2 backpacks.
Whenever you need anything, you need to think in which bagpack something is.

Not only that, but you need to remember the bagpack names too.

One bagpack called gno, and you can autocomplete upon it:

gno.GetBanker()
gno.DerivePkgAddr()

Where's the harm in being simple?

Footnotes

  1. I also like runtime and chain TBH, but they lack the instant, quick reflex quality of "std" or "gno".

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>
@thehowl
Copy link
Member

thehowl commented Sep 20, 2024

I simply advocate renaming std to gno package.

I appreciate the "natural feeling" given by typing out gno.Address, however I do think we should use the term "gno", including in our standard libraries, to refer to the programming language rather than the chain.

This is similar to what happens in go; where the go/* packages are those concerning the Go programming language (ie. parsing, AST, type checker...).

I do agree splitting packages makes sense as possible solution1, however people are not only going to program exclusively in gno in the future.

What?

People aren't going to programming exclusively for gno.land in the future. But this is a change that concerns primarily the standard libraries, which are likely to be brought over into other gno.land ecosystems. This actually strengthens my argument; that chain translates better when using gno elsewhere than having a gno package.

Beginners (in gno) and nobody else are the ones who will become long-term users, and for that conversion rate to be maximized I think they need a simple language.

import "gno" is extremely simple, it's as simple and automatic no-brainer as import "std" today. From there, people get autocompletion upon typing gno.|. It doesn't get more simple for beginners.

good rhetoric ;)

i don't think we'll necessarily end up using it as a keyword; it's a backward-incompatible change with go, i'd much rather use go.

but from principles, i disagree that gno is a better way to namespace types like Address, Coins and Banker. They relate to the blockchain it's referencing, not the programming language.

@Kouteki
Copy link
Contributor

Kouteki commented Oct 16, 2024

@moul
Copy link
Member

moul commented Oct 25, 2024

Tentative reorganization: https://gist.github.com/moul/f28f4b12864fd40a2ca6b6c20294da27

@thehowl thehowl changed the title Discussion: "std" package naming META - "std" name and functions Oct 31, 2024
@thehowl
Copy link
Member

thehowl commented Oct 31, 2024

Combining all the proposals into the top-level comment of this issue

@thehowl
Copy link
Member

thehowl commented Oct 31, 2024

Updated the top-level comment to centralize all proposals.

@moul Please take a look; I tried to summarize also the changes that I tried making with #2425 so that they can be tackled by someone other than me, as we talked about.

What do you think about having a call to go through all of these and figuring out what proposals are sensible and to act on?

I think we can work on these in small PRs, but possibly do all breaking changes ahead of test6.


As a side note, I added the PkgPath / ImportPath problem as well as we talked about it and I think it's still worth a discussion, but I'm more undecided after reading @moul's comment, and especially after I discovered, to my disbelief, that Go in some cases uses PkgPath as well.

@thehowl thehowl changed the title META - "std" name and functions META - splitting and refactoring std Oct 31, 2024
@moul
Copy link
Member

moul commented Oct 31, 2024

Let's schedule a call. Initially, I expected to create a PR instead of this issue so we could comment line by line. However, the problem is that each proposal will not only change lines but also file locations. Therefore, let's have a call.

Regarding whether to create a single PR or multiple ones, my main concern is fixing the history of the portal loop. This depends on #3024 as evidence that we can patch the history for such breaking changes.

Let's assume it will be ready, but we rely not only on developers but also on DevOps to ensure the portal loop loads the Git state. Alternatively, if @gfanton can make gnodev the next portal loop with this feature, that could also work.

@thehowl
Copy link
Member

thehowl commented Nov 7, 2024

Thursday called; reached full consensus on the first three proposals. For the fourth, "Remove std.GetOrigCaller and std.IsOriginCall", let's come back at it next week with more usage examples (both in favour and against the inclusion of the two functions).

@piux2
Copy link
Contributor

piux2 commented Nov 7, 2024

Here are my two cents:

IsOriginCall

We don't need IsOriginCall; instead, we should use AssertOriginCall, as it serves the purpose of eliminating the middleman and removes the risk of not handling errors properly.

GetOrginCaller

We do need to keep GetOriginCaller.

It is used to ensure that the address returned is from an end user's wallet, also known as an external account. The most important security property of GetOriginCaller is that the original message's signature is verified by the chain against GetOriginCaller.

For all non-external callers, such as GetPrevRealm, where the caller is a realm, the contract must trust the caller realm. In use cases where a middle contract needs to be involved, GetOriginCaller acts as a safeguard, delegating trust to the chain rather than the caller contract.

There are two categories of use cases where we need to verify the external account while allowing a middleman contract..

A. Asset (GRC20, NFT) Issued Directly to End Users, with Exchanges or Aggregators Involved

Assets are either directly issued to an end user's wallet or claimed by the end user, who then places them on an exchange or auction house for trading. Aggregators, exchanges, and auction houses are considered middleman contracts.

Example flow:

Original Caller (External Account) → Exchange Aggregator (Third Party A) → Token Exchange (Third Party B) → GRC20 (Token Issuer)

For a safe token withdrawal, exchange contracts need to use GetOriginCaller to ensure that tokens return to the address originally stored in the GRC20 contract, where tokens were directly issued to the end user's wallet address (External Account). In other words, it ensures that the transaction is initiated from a user's origin wallet.

B. Asset Issuers Distributing Assets (GRC20, NFTs, Insurance Policies) via Brokers

Example flow:

Original Caller (External Account) → NFT Broker (Third Party Entity) → GRC721 (NFT Issuer)

NFTs are distributed through a third party as a reward to end users (External Account). The NFT issuer contract needs to verify that the NFT is claimed from an external account.

C: Permissioned Use Cases Combining GetOriginCaller and GetPrevRealm

For permissioned scenarios, we can combine GetOriginCaller and GetPrevRealm to whitelist middleman contracts. GetOriginCaller ensures that the transaction is initiated by an end user, while GetPrevRealm ensures the transaction is from a whitelisted middleman.

@leohhhn The issue you mentioned with other chains is a bit different because they don't have AssertOriginCall built into the language to eliminate the middleman contract. They only have something similar to GetOriginCaller and GetPrevRealm. For contracts that actually verify direct withdrawals from external accounts while involving a legitimate middleman contract, developers have to write something like GetOriginCaller == GetPrevRealm in their contracts.

Other thoughts.

I see the complication arises from GetPrevRealm returning the same information as GetOriginCaller in direct call cases.

In abstract terms, we can consider a user's external account as a realm, especially when MsgRun is involved, in which the external account signs and executes a temporary realm contract called "main."

However, in reality, GetPrevRealm returns user information that overlaps with other functions, and it cannot replace GetOriginCaller or AssertOriginCall.

When verifying the OriginCaller, there are a few design options:

  • Eliminate the Middleman

    Option 1: GetOriginCaller + AssertOriginCall
    Option 2: PrevRealm.IsUser + PrevRealm.Address + AssertOriginCall
    Option 3: PrevRealm.IsUser + GetOriginCaller + other logic to handle the negative case
    Option 4: PrevRealm.IsUser + PrevRealm.Address + other logic to handle the negative case
    Option 5: GetOriginCaller==PrevRealm.Address + PrevRealm.Address + other logic to handle the negative case

    There is one specific case. Using PrevRealm in Option 6 seems to simplify verifying if an EOA is calling a contract as an admin. However, it’s less trustworthy because users can’t be sure if the admin address is truly an EOA or another contract. Only the contract creator knows if intermediaries are involved, leaving others unable to fully trust this assumption.

    Option 6: PrevRealm.Address==g1jg8m........

  • Allow Whitelist the Middleman

    GetOriginCaller, PrevRealm.IsRealm, PrevRealm.Address

  • Allow Permissionless Middleman

    GetOriginCaller

As we can see, PrevRealm.IsUser is only useful in the first case, where GetOriginCaller + AssertOriginCall is the cleaner option.

What if we remove the mental burden that PrevRealm could also return a user address? Instead, PrevRealm either returns a realm on-chain or it doesn't.

This way, everything is clean and straightforward:

  • Eliminate the Middleman

    GetOriginCaller + AssertOriginCall

  • Allow Whitelist the Middleman

    GetOriginCaller + PrevRealm.OK + PrevRealm.Address

  • Allow Permissionless Middleman

    GetOriginCaller

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

10 participants