-
Notifications
You must be signed in to change notification settings - Fork 393
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
refactor: r/demo/users
#3166
base: master
Are you sure you want to change the base?
refactor: r/demo/users
#3166
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
# Conflicts: # examples/gno.land/r/demo/users/users.gno # examples/gno.land/r/demo/users/z_10_filetest.gno # examples/gno.land/r/demo/users/z_11_filetest.gno # examples/gno.land/r/demo/users/z_11b_filetest.gno # examples/gno.land/r/demo/users/z_2_filetest.gno # examples/gno.land/r/demo/users/z_3_filetest.gno # examples/gno.land/r/demo/users/z_4_filetest.gno # examples/gno.land/r/demo/users/z_5_filetest.gno # examples/gno.land/r/demo/users/z_6_filetest.gno # examples/gno.land/r/demo/users/z_7_filetest.gno # examples/gno.land/r/demo/users/z_7b_filetest.gno # examples/gno.land/r/demo/users/z_8_filetest.gno # examples/gno.land/r/demo/users/z_9_filetest.gno # examples/gno.land/r/sys/users/verify.gno
u := *(data.(*UserData)) | ||
if u.deleted { | ||
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.
u := *(data.(*UserData)) | |
if u.deleted { | |
return nil | |
var u *UserData | |
*u = *(data.(*UserData)) | |
if u.deleted { | |
return nil |
should be better (please, verify in a test)
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.
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.
a5a84af -> not good; should be a copy; did you try my snippet above?
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.
Fixed it: a5b8f43
out += md.H2("User list") | ||
|
||
p := pager.NewPager(users.GetReadOnlyNameStore(), 20, false) | ||
page := p.MustGetPageByPath(path) | ||
for _, item := range page.Items { | ||
if item.Value == nil { | ||
continue | ||
} | ||
|
||
data := item.Value.(*users.UserData) | ||
|
||
// Skip previous names | ||
if item.Key != data.Name() { | ||
continue | ||
} | ||
|
||
out += ufmt.Sprintf("- User [%s](/r/gnoland/users/v1:%s)\n", md.Bold(item.Key), item.Key) | ||
} |
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.
Even though I'm glad you're using this pattern that I was proud of (thank you, @leohhhn), I believe this realm will quickly become unusable because it will contain:
- aaa000
- aaa001
- aaa002
- etc.
It makes more sense to either print statistics or eventually set up an efficient ring buffer for the last N entries (we don't have a good data structure yet, so let's create a p/ for this :p). In the meantime, just store the last created *UserData entry.
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.
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.
I finally went over with this review; it looks good; ping me when you've addressed the last feedback 👍
@@ -30,7 +30,7 @@ func init() { | |||
// add pre-registered users | |||
for _, res := range preRegisteredUsers { | |||
if err := users.RegisterUser(res.Name, res.Address); err != nil { | |||
panic(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.
_ users.RegisterUser(...
seems better than a if { continue }
; or maybe keep the if and emit/log something.
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.
@@ -3,14 +3,9 @@ package users | |||
import ( | |||
"std" | |||
|
|||
"gno.land/p/demo/users" | |||
"gno.land/r/sys/users" |
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.
"gno.land/r/sys/users" | |
susers "gno.land/r/sys/users" |
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.
Description
Addresses: #2827
This PR refactors the current
r/demo/users
&r/sys/users
system in accordance to the issue mentioned above. Below is the high-level overview of the new system.It also makes a small change to the
p/demo/releases
lib.r/gnoland/users/v1
r/demo/users
r/sys/users
Mainnet v1 (mvp) & v2 #2827r/demo/profile
for displaying extra information about a user (to be moved tor/nt/profiles
orr/gnoland/profiles
)r/sys/users
r/sys/names
r/gnoland/users/v1
, etc.r/gov/dao/bridge
#3523 was resolved)r/sys/names
r/sys/users
), added tests, as discussed with @moulVerify functionality is now not pausable and on by default, as per discussions with @moul. This means Portal Loop will need to be patched upon merging this PR, as many txs will fail due to not having proper namespace permissions.The namespace verify functionality is off by default, but should be enabled in genesis viar/sys/names.Enable()
. The Portal Loop will indeed need to be patched if we enable this functionality.Keeper
& genesis paramsr/sys/names
for the namespace check instead ofr/sys/users
r/sys/names
, which the keeper will callr/gnoland/users/v1
r/gnoland/users
Currently this is the "releases" page for the user registration systems (example data):
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description