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: r/system/names public functions and checks with AddPackage #384

Closed
wants to merge 20 commits into from

Conversation

anarcher
Copy link
Contributor

@anarcher anarcher commented Oct 30, 2022

  • r/system/names public functions
  • r/system/names tests
  • vmkeeper.AddPackage uses r/system/names for checks(ACLs)

refs: #375

image

image

@anarcher
Copy link
Contributor Author

anarcher commented Nov 5, 2022

It's not perfect, but it adds some basic functionality.
To check namespace permission in vmkeeper.AddPackage, I used vm.Call in AddPackage.

	res, err := vm.Call(ctx, MsgCall{
		Caller:  creator,
		Send:    std.Coins{},
		PkgPath: "gno.land/r/system/names",
		Func:    "HasPerm",
		Args:    []string{namespace},
	})
$  gnokey maketx addpkg anarcher  --pkgdir .  --gas-fee "1ugnot"  --gas-wanted "5000000"  --broadcast true  --pkgpath "gno.land/r/test_1/hello3"
Enter password.
namespace "test_1" not allowed

@anarcher anarcher changed the title Draft: feat: r/system/names public functions and checks with AddPackage feat: r/system/names public functions and checks with AddPackage Nov 5, 2022
@anarcher
Copy link
Contributor Author

anarcher commented Nov 5, 2022

1819bf7

In GeneisisDoc, r/system/names should add last than others. :-)

@anarcher anarcher marked this pull request as ready for review November 8, 2022 15:03
@moul moul added this to the 🏗4️⃣ test4.gno.land (next) milestone Nov 9, 2022
func RemoveAdmin(namespace string, admin std.Address) {
space := getSpace(namespace)
assertIsAdmin(space)
err := space.removeAdmin(admin)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
err := space.removeAdmin(admin)
// TODO: prevent self
err := space.removeAdmin(admin)

I suggest we remove the possibility of demoting ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does that mean the caller can't remove the caller himself?

Copy link
Member

Choose a reason for hiding this comment

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

Caller can remove itself by creating another account, promoting it as admin, and the new account to demote the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It checks to prevent self removing.
dac208f

@piux2
Copy link
Contributor

piux2 commented Nov 27, 2022

Does this mean only people on the r/system/names/names.gno can call Add Package to the gno.land realm and Admins have full control of the name. gno contract?

If that is the case, it becomes a permissioned contract(realm) space. Are we sure this is what we want?

@anarcher
Copy link
Contributor Author

Does this mean only people on the r/system/names/names.gno can call Add Package to the gno.land realm and Admins have full control of the name. gno contract?

If that is the case, it becomes a permissioned contract(realm) space. Are we sure this is what we want?

Anyone can add namespaces (permissionless). IMHO I think that namespace is a collection of contracts(realms).

@piux2
Copy link
Contributor

piux2 commented Nov 27, 2022

Does this mean only people on the r/system/names/names.gno can call Add Package to the gno.land realm and Admins have full control of the name. gno contract?
If that is the case, it becomes a permissioned contract(realm) space. Are we sure this is what we want?

Anyone can add namespaces (permissionless). IMHO I think that namespace is a collection of contracts(realms).

We have the same understanding.
I think namespace is also the path to a contract on gnoland, as a chain. just like github URL directory.
the package path(naming space) is mapped to the package address,

vmkeeper.AddPackage requesting permission from contracts under gno.land/r/system/names prevent people from deploying contracts with their naming space permissionless.

Here are my 2 cents.
If we model gnoland as a github instead of a computer, the contract under r/gnoland/system is better to control gno.land website parameters and functions instead of controlling the gnoland as the chain

@anarcher
Copy link
Contributor Author

the contract under r/gnoland/system should not control the chain since every contract is center controlled by admins.

Each contract is controlled by its own admins.
IMHO, managing /r/systems/names contract itself (and other contracts in /r/system?) should be need gov tool (review?) or something else.

@piux2
Copy link
Contributor

piux2 commented Nov 27, 2022

the contract under r/gnoland/system should not control the chain since every contract is center controlled by admins.

Each contract is controlled by its own admins. IMHO, managing /r/systems/names contract itself (and other contracts in /r/system?) should be need gov tool (review?) or something else.

Yes, Each contract is controlled by its own admins.

But who can call AddPkg, and paths to the Pkg are controlled by the realm contract r/system/name. This is not permissionless unless we want a group of admin gov the entire naming space on gnoland as a chain.

The contract admin controls smart Contract logic and the privilege to update the contract and modify the state.

For other chains out there, using the smart contract to manage (gov) chain parameters is not a design choice but a workaround for VM and chain design limitations. But in Cosmos-SDK-based GVM, we don't have such constraints. There is another option: Instead of checking gno.land/r/system/names for ownership of a namespace in VMKeeper, We could check in the Pkg Base account on a first-come-first-serve basis and keep the reserved name in the genesis file instead of /r/system/names/genesis.gno


This is called in VMKeeper.AddPkg(). It means everyone's contract path on gnoland is controlled by this realm contract gno.land/r/system/names

PkgPath: "gno.land/r/system/names",
Func:    "HasPerm",
// - lookup r/system/names.namespaces for `{r,p}/NAMES`.

in checkNamespacePerm()

func (vm *VMKeeper) checkNamespacePerm(ctx sdk.Context, creator crypto.Address, pkgPath string) error {
store := vm.getGnoStore(ctx)

// if r/system/names does not exists -> skip validation.
if pv := store.GetPackage("gno.land/r/system/names", false); pv == nil {
	return nil
}

pathSp := strings.SplitN(pkgPath, "/", 4) // gno.land/r/...
namespace := pathSp[2]

res, err := vm.Call(ctx, MsgCall{
	Caller:  creator,
	Send:    std.Coins{},
	PkgPath: "gno.land/r/system/names",
	Func:    "HasPerm",

@moul
Copy link
Member

moul commented Nov 28, 2022

For gno.land, we'll act like GitHub, which means people will register namespaces for themselves or their organizations; then, they'll be able to push contracts in the namespace.

The reservation of handle won't be approved by admins but by the r/system/names contract, which should have at least:

  • an automatic reservation system that follows the rules, i.e., paying fees like r/demo/users with different rules.
  • some ways to moderate to avoid bad squatting, i.e., someone reserving all the brands.
  • ways to reserve/blacklist some names for the system/brand/moderation/security/whatever.

If the system is not fully automated, it will be managed by the DAO of contributors, not by an admin.
The current admins (Jae and I) should not get any extra advantages at the end; we'll be able to vote and probably have more tokens but no additional power.

About permissionless, we've multiple options:

  1. allow registration of any name with some criteria, i.e., a prefix, more than 10 chars, proving a GitHub account, whatever.
  2. same as 1. with transparent/lazy registration.
  3. not a fan at all, but eventually, we could have instances of gno focused on production/sandboxing/discovery; I think this is a bad idea.

About the definition of a namespace, it's:

  • The first part after p/ and /r/, so a prefix for smart contracts.
  • Something that can be queried by other contracts like r/demo/users with more official links with the contributions (i.e., being part of multiple organizations).
  • A way to manage current teamates.
  • A way to identify a namespace member from an external contract, i.e., for ACLs.
  • Probably related to future proof-of-contributions (not sure yet).
  • Tomorrow, probably more things around the user/team like a GitHub profile which contains more than just a list of repos.

@piux2
Copy link
Contributor

piux2 commented Nov 28, 2022

Overall is excellent thinking and design.

There is a critical piece we want to discuss.

r/system/names. the contract will need a group of admin (DAO) with threshold multi-sig. Then, they can update the contract, which could change the state of the namespace stored in the contract's ival tree.

These namespaces are three things together.

  1. package path, where people can refer to the source code or import from other code.
  2. contract package address, where VM can identify the contract account
  3. contract API endpoint, where web front and off-chain services can call upon.

Using a contract (r/system/names) to manage these elements in a chain degrades its security and trust; many other EVM-based chains do that because they don't have other choices.

Managing the entire gnoland blockchain from a contract is the opposite of the AppChain, and the Gov value model originated from cosmos-SDK.

Why don't we separate the concept of gnoland as blockchain and gno.land as a web3 application that interfaces the underline gnoland chain?

Let Dao contract manage contracts published under r/gnoland/ only?

Essentially, we are making trade-off decisions between preventing abusing namespace vs providing permissionless trust.

We want to explore more on preventing abusing namespace without degrading permissionless trust, which is the core value

@moul
Copy link
Member

moul commented Nov 28, 2022

The plan is to use a DAO to administer most of the critical parts of the target. You can consider that the DAO of all contributors will manage r/system/names.

About updating the contract, we've multiple options. One of the most promising ones is to support package upgrades with some built-in symlinks. So we could say that members of the r/system's team (which could be the DAO, maybe?) would be able to publish new versions of the contract and then update the symlink.

Using a contract (r/system/names) to manage these elements in a chain degrades its security and trust; many other EVM-based chains do that because they don't have other choices.
Managing the entire gnoland blockchain from a contract is the opposite of the AppChain, and the Gov value model originated from cosmos-SDK.
Why don't we separate the concept of gnoland as blockchain and gno.land as a web3 application that interfaces the underline gnoland chain?

We will have two phases: testnets and mainnet.

During testnets, we'll make multiple iterations:

  • first one is free-for-all -> r/system/names can just allow anyone to register anything. fully permissionless.
  • second one -> r/system/names works with some kind of DAO/invite system BUT we keep a dedicated room for free uploads, i.e., free names with a recognizable prefix (unverified-blahblah); or a special namespace r/unverified/* that won't check r/system/names
  • ...

Mainnet will be different, we want better quality, so it will probably be a mix of:

  • automated registration if people pay a lot of fees.
  • manual registration with approval by humans for the other ones.
  • maybe we can also make things to link and give for free a username as soon as you can prove the ownership of a github account.
    A.k.a., flexible permissionned.
    But the thing is that we'll also have testnets, or maybe other mainnets. Gno.land is not about making a chain for everyone, there will be many places to write gno code; Gno.land will be like GitHub, a place where you want to build a trust relationship with your users by publishing multiple contracts. It will be intereconnected with other chains including more flexible ones too.

Why don't we separate the concept of gnoland as blockchain and gno.land as a web3 application that interfaces the underline gnoland chain?

gno.land is the blockchain; it's not a standalone website, it's the built-in website. people can do other stuff on other frontends, but this one is intrinsically linked with the blockchain; and the current philosophy is to keep it very close to the low-level chain.

We want to explore more on preventing abusing namespace without degrading permissionless trust, which is the core value

We want spam prevention and quality AND we want permissionless, but not necessarily on the same location. It's OK Gno.land, the chain that will stay for 100y, powering many other chains including Gnolang chains is more limited to require people to buy a namespace or make an IRL proof or other registration flow. While letting anyone use more permissionless instances of Gnolang.

Gno.land shouldn't be the only place for everything Gno, Gno.land is the Cosmos-Hub of the Gno ecosystem.
We can have official sister chains that are well linked with gno.land, and official too, but with different rules and maybe PoS instead of PoC.

@piux2
Copy link
Contributor

piux2 commented Nov 29, 2022

So we could say that members of the r/system's team (which could be the DAO, maybe?) would be able to publish new versions of the contract and then update the symlink.

Great idea. The dots start connecting.

We are missing a component, which is the Gov module. Using gov voting to control updating the r/system contracts will reduce the trust and security concerns on admin access to the smart contracts to the minimum.

@moul
Copy link
Member

moul commented Jan 9, 2023

In order to merge this one and not block people that work on localnet etc, we should make the contract disabled by default (even if published); and make it easy to toggle the contract later.

@piux2
Copy link
Contributor

piux2 commented Jan 10, 2023

In order to merge this one and not block people that work on localnet etc, we should make the contract disabled by default (even if published); and make it easy to toggle the contract later.

An alternative way to solve the security issue is to completely remove the Admin in the contract and put the contract deployment in the genesis state. The system parameter contracts can only be upgraded through coordinated chain upgrades, which require the gov approval by default.

@anarcher
Copy link
Contributor Author

In order to merge this one and not block people that work on localnet etc, we should make the contract disabled by default (even if published); and make it easy to toggle the contract later.

I agree that. What type of disable/enable method do we need?  :-D. Let's take a simple approach first?
var enable bool In r/system/names and dev/testnet genesis doesn't have code like this func init() { enable = true } ?

@github-actions github-actions bot removed the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jul 21, 2023
@anarcher
Copy link
Contributor Author

anarcher commented Jul 21, 2023

Hey @anarcher, sorry for the very late reply. I'm setting up review queues to make sure that we're up to speed with very old PRs and reviewing anything that hasn't actually been reviewed yet.

Thank you again for your contribution and patience. I'm putting this on the top of my review queue.

Since it took so long, however, things have changed a bit from master so I'd ask you to merge upstream/master; you'll need to remove the changes in gnoland/main.go and maybe update the gno.mod file in r/system/names.

Thank you! ✨

I did rebase with master, and updated gno.mod. Is there a cmd like gno.mod tidy planned or is there any? :)
Thank you!

@moul
Copy link
Member

moul commented May 26, 2024

Based on recent discussions, I think this PR is mostly good and we should try recycling it for #1107 (cc @gfanton).

See also #2205.

@piux2
Copy link
Contributor

piux2 commented Jul 31, 2024

@moul @anarcher I think we should remove name space under r/system/ out of the space permission in this contract to provide better security and trust. This contract can only manage the name space out side of r/system. I would suggest only loading system contract from genesis without admin. Only upgrade path is through chain upgrade.

@@ -12,49 +14,71 @@ import (
// determine if an address can publish a package or not.
var namespaces avl.Tree // name(string) -> Space

// TODO: more accurate.
var reNamespace = regexp.MustCompile(`^[a-z][a-z0-9_]{2,30}$`)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove the /r/system from the name space here to prevent adding admin to the /r/system contracts

@thehowl
Copy link
Member

thehowl commented Jul 31, 2024

@piux2 this PR should have been closed - please make your comments elsewhere, maybe as an issue. It has been continued with #2471 and merged, and is live on test4.

@thehowl thehowl closed this Jul 31, 2024
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 🧾 package/realm Tag used for new Realms or Packages. 🌱 feature New update to Gno
Projects
Status: Done
Status: ✅ Done
Status: 🚀 Needed for Launch
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants