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: support realm subpackages #365

Merged
merged 8 commits into from
Oct 27, 2022
Merged

Conversation

anarcher
Copy link
Contributor

@anarcher anarcher commented Oct 20, 2022

Large program (included smart contracts?) has constructed with packages. Like normal packages (gno.land/p), This PR is for realm sub(nested) packages like below.

upgrade-a
├── integration_test.gno
├── package.go
├── v1
│   └── v1.gno
└── v2
    └── v2.gno

IMHO, Subpackages can be helpful in the versioned or upgradable realm.
For realm's ownership, A creator should be equal to root realm(gno.land/r/<root>) and sub realms (gno.land/r/<root>/<sub>). So I added PackageAccount.Owner to GnoAccount.

This is a realm package account

$ gnokey query auth/accounts/g1pzll2zyn0rrcshefa25k8v50annqr6lew80pq2
height: 0
data: {
  "BaseAccount": {
    "address": "g1pzll2zyn0rrcshefa25k8v50annqr6lew80pq2",
    "coins": "",
    "public_key": null,
    "account_number": "76",
    "sequence": "0"
  },
  "PackageAccount": {
    "owner": "g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5"
  }
}

This is an normal account.

height: 0
data: {
  "BaseAccount": {
    "address": "g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5",
    "coins": "9999980000000ugnot",
    "public_key": null,
    "account_number": "0",
    "sequence": "0"
  }
}
$  gnokey maketx addpkg anarcher --pkgpath "gno.land/r/hello10/v1" --pkgdir .  --gas-fee "1ugnot"  --gas-wanted
"5000000"  --broadcast true
Enter password.

OK!
GAS WANTED: 5000000
GAS USED:   356358

@moul
Copy link
Member

moul commented Oct 22, 2022

Thank you very much;

I need to figure out the best way to combine your PR and #336.

But that’s clearly going in the good direction.
👍

@moul moul self-requested a review October 22, 2022 11:03
Comment on lines 167 to 173
// Add package account.
pkgAcc := vm.acck.NewAccountWithAddress(ctx, pkgAddr)
ga := pkgAcc.(*types.GnoAccount)
ga.PackageAccount = &types.PackageAccount{
Owner: creator,
}
vm.acck.SetAccount(ctx, ga)
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about removing this part and make the creator != rootAcc.Owner recompute the parent realm’s author dynamically?

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

Hey @anarcher

Thank you very much for this PR. It goes precisely in the good direction.

To be more future-proof, where a contract developer will not have only one top-level realm but multiple ones, including shared ones for organizations. I’ll ask you to revert the extended account with the “owner” part and make runtime computing for permissions.

So basically, only keep (and adapt):

  • pgks/std/memfile.go
  • pkgs/sdk/vm/keeper.go

In an upcoming PR, I'll add support for realm reservation, linked with a new system-wide namespace realm + team support. And I will extend your work.

@moul moul added this to the 🏗3️⃣ test3.gno.land milestone Oct 25, 2022
@anarcher
Copy link
Contributor Author

Thanks for kind feedback @moul :-)

To be more future-proof, where a contract developer will not have only one top-level realm but multiple ones, including shared ones for organizations. I’ll ask you to revert the extended account with the “owner” part and make runtime computing for permissions.

I like your idea(runtime permission). :) IMHO, I think one of advantages of runtime permission is that many contract developers can add sub realms of one root realm (and adding/removing contract developer dynamically). Is it right?

I will revert the extended account with the “owner” part and then making runtime permission.

@anarcher
Copy link
Contributor Author

Hi @moul,
For runtime permissions, My first thought was to maker/realm/acl using in vmkeeper.AddPackage. But this approach raise some kind of chicken-eggs problem or packages loading order. (Because we should add pkgs after r/realm/acl is loaded for checking permission and we should add r/realm/acl without r/realm/acl. :-)

What do you think about? Do you have better idea? :-)

@moul
Copy link
Member

moul commented Oct 27, 2022

Hey @anarcher,

I appreciate that you want to help, but I prefer that we split the topic:

My idea is similar to yours but without the chicken-eggs problem :)

@anarcher
Copy link
Contributor Author

anarcher commented Oct 27, 2022

I revert PackageAccount. :-) The rest of the code is just the part of allowing subpackage paths.

My idea is similar to yours but without the chicken-eggs problem :)

👍 I'm curious about your idea :-)

@anarcher
Copy link
Contributor Author

anarcher commented Oct 27, 2022

Another my idea about it is that each top-realm(namespace?) has own acl.gno. I think that it doesn't make the chicken-eggs problem. vmkeeper.AddPackage checks top path or sub path. If sub realm are added, vmkeeper.AddPackage just call HasPerm() in root realm of this sub realm. What do you think about it? :-)

@moul moul changed the title Support realm subpackages feat: support realm subpackages Oct 27, 2022
@moul moul merged commit 789c909 into gnolang:master Oct 27, 2022
@moul
Copy link
Member

moul commented Oct 27, 2022

Thank you!

I’ll rebase and add new comments on my other PR so you can contribute too if you want :)

@moul
Copy link
Member

moul commented Oct 28, 2022

Please, check out #375

@moul moul mentioned this pull request Oct 28, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants