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: add realm r/demo/keystore #958

Merged
merged 11 commits into from
Sep 16, 2023
Merged

feat: add realm r/demo/keystore #958

merged 11 commits into from
Sep 16, 2023

Conversation

schollz
Copy link
Contributor

@schollz schollz commented Jul 7, 2023

This realm implements the avl.Tree as a user-addressable key-store. I wanted to have this realm to possibly use gno.land as a database where writes may cost gnots but reads are free because they can be performed through Render (related discussion: #947). As a nice additional feature it provides a UI for the database (through gno.land).

Data can be set/removed if the caller is the same as the owner of the key-store (i.e. only owner's have write-access). For example:

# Set key,value for user at YOURKEY (write-protected)
gnokey maketx call --pkgpath "gno.land/r/demo/keystore" \
--func "Set" --args "hello" --args "world" \
--gas-fee "1000000ugnot" --gas-wanted "8000000" \
--broadcast --chainid dev --remote localhost:26657 YOURKEY
# Remove key for user at YOURKEY (write-protected)
gnokey maketx call --pkgpath "gno.land/r/demo/keystore" \
--func "Remove" --args "hello"  \
--gas-fee "1000000ugnot" --gas-wanted "8000000" \
--broadcast --chainid dev --remote localhost:26657 YOURKEY
# Get key
gnokey maketx call --pkgpath "gno.land/r/demo/keystore" \
--func "Get" --args "hello"  \
--gas-fee "1000000ugnot" --gas-wanted "8000000" \
--broadcast --chainid dev --remote localhost:26657 YOURKEY

All data is public and accessible as read-only from any user, as well as from gno.land.

The main page lists all the available databases by owner (/r/demo/keystore):

image

Clicking on a user will show all the keys in their key-store (/r/demo/keystore:USER):

image

And the key values are accessible as well (r/demo/keystore:USER:get:KEY)

image

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.

Maintainers Checklist

  • Checked that the author followed the guidelines in CONTRIBUTING.md
  • Checked the conventional-commit (especially PR title and verb, presence of BREAKING CHANGE: in the body)
  • Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

this realm provides a way to interact with the avl.Tree
through gno.land as well as through transactions so that
users can have individualized keystores.
@schollz schollz requested a review from a team as a code owner July 7, 2023 17:59
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jul 7, 2023
@harry-hov harry-hov self-requested a review July 7, 2023 23:35
@ajnavarro
Copy link
Contributor

Hi @schollz! do you mind adding the checks from the PR template? thanks!

@schollz
Copy link
Contributor Author

schollz commented Jul 10, 2023

Yes. Sorry about that @ajnavarro , will do so

@schollz
Copy link
Contributor Author

schollz commented Jul 10, 2023

Checklist added! Looks like I'm missing benchmarks...let me know if that's something that should be added.

@ajnavarro
Copy link
Contributor

Thanks a lot, @schollz , if you didn't add any new benchmarks, you can check that too. These are just reminders of things that we usually miss but they help with the reviews and to keep the repo up and running.

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looks good 💯

I've left a few comments that would simplify the code.

I love the idea of using this as a DB for user public keys, I think it eases that part of knowledge sharing

Comment on lines 13 to 21
var (
BaseURL = "/r/demo/keystore"
StatusOK = "ok"
StatusNoUser = "user not found"
StatusNotFound = "key not found"
StatusNoWriteAccess = "no write access"
StatusCouldNotExecute = "could not execute"
)

Copy link
Member

Choose a reason for hiding this comment

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

These can be constants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed in 76b7131

Comment on lines 42 to 46
origOwner := std.GetOrigCaller()
if origOwner.String() != owner {
return StatusNoWriteAccess
}
keystoreInterface, exists := data.Get(owner)
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 extracting this out into a specific helper method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm of the mind to leave it - it only occurs twice in the code and moving out into a function may only reduce the code by a few lines.

Comment on lines 48 to 56
data.Set(owner, &KeyStore{
Owner: origOwner,
Data: avl.Tree{},
})
keystoreInterface, _ = data.Get(owner)
}
keystore := keystoreInterface.(*KeyStore)
keystore.Data.Set(k, v)
return StatusOK
Copy link
Member

Choose a reason for hiding this comment

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

Why are you setting the keystore, and then fetching it again from the map (avl)?

Why not simply define a var keystore KeyStore outside the condition, and if it doesn't exist, assign it in the branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right, thanks for that. Its fixed in 61079bb

Comment on lines 75 to 82
data.Set(owner, &KeyStore{
Owner: origOwner,
Data: avl.Tree{},
})
keystoreInterface, _ = data.Get(owner)
}
keystore := keystoreInterface.(*KeyStore)
_, removed := keystore.Data.Remove(k)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 61079bb

Comment on lines 132 to 134
// "owner:set:key:value" -> sets a key-value pair for owner's keystore (owner must be caller)
// "owner:remove:key" -> removes key from owner keystore (owner must be caller)
func Render(p string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep Render to be read-only?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, usage of Render() should probably be universally understood as read-only for consistency across realm examples though there isn't anything in the docs stating this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Was not aware that Render() should be read-only. Fixed in dd50037

args := strings.Split(p, ":")
numArgs := len(args)
if p == "" {
if data.Size() > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

You can simplify this code segment by:

if data.Size() == 0 {
    return "no databases"
}

data.Iterate(...)
// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I do prefer that pattern. Fixed in 292dc1e

var response string
args := strings.Split(p, ":")
numArgs := len(args)
if p == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Why not use a switch, instead? You have return logic for each branch, and only one branch can execute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah! I'm not a huge fan of switches but I agree its nice here, fixed in 8854a13

@moul moul requested a review from waymobetta August 16, 2023 16:10
@zivkovicmilos
Copy link
Member

Ping @schollz for visibility

@moul moul added this to the 🌟 main.gno.land (wanted) milestone Sep 8, 2023
@moul moul merged commit ae577ec into gnolang:master Sep 16, 2023
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants