- Notifications
You must be signed in to change notification settings - Fork397
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
feat(examples): awesome gno#3963
base:master
Are you sure you want to change the base?
Conversation
🛠 PR Checks Summary🔴 Pending initial approval by a review team member, or review from tech-staff Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report?Let us know! |
Problem here with "not enough arguments in call to page.Picker", same as#3793 (comment) :-) |
Addressed in567df81. I've also added hof registration (if you remember#3479 it caused lint to break in init) which suprisingly causes no trouble here.. |
| admins.Set("g1ej0qca5ptsw9kfr64ey8jvfy9eacga6mpj2z0y", true) | ||
| admins.Set(std.CurrentRealm().Address().String(), true) | ||
| admins.Set(std.PreviousRealm().Address().String(), true) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you can usep/moul/addrset for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey I figured since authz is already managing admin set so I've just implemented that instead in048d3ef
| func AddAdmin(addr std.Address) error { | ||
| caller := std.PreviousRealm().Address() | ||
| if !isAdmin(caller) { | ||
| return ufmt.Errorf("not authorized: only admins can add new admins") | ||
| } | ||
| if !addr.IsValid() { | ||
| return ufmt.Errorf("invalid address") | ||
| } | ||
| admins.Set(addr.String(), true) | ||
| std.Emit("AdminAdded", "address", addr.String()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be handled with authorizable or with p/moul/authz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
applied in048d3ef
| func RemoveAdmin(addr std.Address) error { | ||
| caller := std.PreviousRealm().Address() | ||
| if !isAdmin(caller) { | ||
| return ufmt.Errorf("not authorized: only admins can remove admins") | ||
| } | ||
| if !addr.IsValid() { | ||
| return ufmt.Errorf("invalid address") | ||
| } | ||
| if admins.Size() <= 1 { | ||
| return ufmt.Errorf("cannot remove the last admin") | ||
| } | ||
| admins.Remove(addr.String()) | ||
| std.Emit("AdminRemoved", "address", addr.String()) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Same as above, use a package for privileged actions. I suggest you dig into p/moul/authz; we are slowly moving away from ownable and authorizable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
applied in048d3ef
| categories.Iterate("", "", func(key string, value interface{}) bool { | ||
| cat := value.(Category) | ||
| if cat.Name == name { | ||
| exists = true | ||
| return true | ||
| } | ||
| return false | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Definitely not like this; you should use the fact that AVL tree has a lookup of O(1) - just use the category name as the key. Make sure that you sanitize the key properly (ie no spaces,etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also you can useany instead ofinterface{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yeah, makes sense, forgot aboutavl.Has().. implemented in4f6f4fa
| DappIDs: []uint64{}, | ||
| } | ||
| categories.Set(strconv.FormatUint(nextCategoryID, 10), category) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FormatUint >strconv.Itoa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this mean I should generally useFormatUint ?
| nextDappID uint64 = 1 | ||
| nextProposalID uint64 = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
seqid is more practical when working with AVL trees
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok implemented in9887784
| func ListCategories() []Category { | ||
| result := []Category{} | ||
| categories.Iterate("", "", func(key string, value interface{}) bool { | ||
| result = append(result, value.(Category)) | ||
| return false | ||
| }) | ||
| return result | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you need this to be a getter for someone outside of this realm, check out avl/rotree, which exposes a tree without setter functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, that's what I was going for (but also for render), applied in6fe1d53
| return result | ||
| } | ||
| func ProposeNewDapp(title, description, url string, categoryNamesStr string) (uint64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For proposals, let's try using some DAO code that already exists.@jeronimoalbi maybe we can integrate with gno.me as we talked, but maybe we should just do commondao+datasource. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Using gno.me seems to be the right approach with the caveat that before mainnet will require some small changes to your custom proposal definition methods, there are a couple of changes to be done for security that are still pending. But this way would be interesting so you can have a SubDAO within the gno.me DAO tree.
Using commondao is also valid, as a package or directly though the commondao realm. The package would require a bit more effort. If commondao is used we can also integrate with gnome space if it makes sense and list content there if you support thedatasource interface fromgno.land/p/jeronimoalbi/datasource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey, so in381c72d I've added datasource support for Proposals, Dapps and Categories (with tests) as we talked today@leohhhn, I'd love some feedback on that as it is pretty long. Looking forward, I should wait until commonDao package/realm is merged and use that here if I understood our conversation today correctly?
| var categoryNames []string | ||
| if categoryNamesStr != "" { | ||
| for _, name := range strings.Split(categoryNamesStr, ",") { | ||
| trimmedName := strings.TrimSpace(name) | ||
| if trimmedName != "" { | ||
| categoryNames = append(categoryNames, trimmedName) | ||
| } | ||
| } | ||
| } | ||
| if len(categoryNames) > 0 { | ||
| for _, catName := range categoryNames { | ||
| categoryExists := false | ||
| categories.Iterate("", "", func(key string, value interface{}) bool { | ||
| cat := value.(Category) | ||
| if cat.Name == catName { | ||
| categoryExists = true | ||
| return true | ||
| } | ||
| return false | ||
| }) | ||
| if !categoryExists { | ||
| return 0, ufmt.Errorf("category '%s' does not exist", catName) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Too much nesting, can be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done in29f82c1
| caller := std.PreviousRealm().Address() | ||
| if !isAdmin(caller) { | ||
| return ufmt.Errorf("not authorized: only admins can update dApp categories") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this error repeats, it should be declared somewhere. speciifcally for this, either use the p/ errors for control access (ownable/authz), or declare it at top level of this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this commit defines errors in types.gno and uses those
| for _, catName := range strings.Split(newCategories, ",") { | ||
| _, found := GetCategory(catName) | ||
| if !found { | ||
| return ufmt.Errorf("category not found: %s", catName) | ||
| } | ||
| } | ||
| for _, oldCat := range dapp.Categories { | ||
| found := false | ||
| for _, newCat := range strings.Split(newCategories, ",") { | ||
| if oldCat == newCat { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| err := RemoveDappFromCategory(dappID, oldCat) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| for _, newCat := range strings.Split(newCategories, ",") { | ||
| found := false | ||
| for _, oldCat := range dapp.Categories { | ||
| if newCat == oldCat { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| err := addDappToCategory(dappID, newCat) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Definitely too much nesting.
If you need to use double for loops, you're probably doing something wrong - these could be avoided with the proper use of maps/avl trees
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes i see what you mean, hopefully this makes more sense :27dc704
| caller := std.PreviousRealm().Address() | ||
| if !isAdmin(caller) { | ||
| return ufmt.Errorf("not authorized: only admins can delete dApps") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same comment as above for errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
4cd9637 this and all other non specific errors
implementation of#3928
This is an on chain representation of the Awesome Gno repo which serves a purpose of showcasing dApp built in the community. The way this implementation works is:
UI (mock data):