-
Notifications
You must be signed in to change notification settings - Fork 378
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
chore(demo): upgradable realm demo #3147
base: master
Are you sure you want to change the base?
chore(demo): upgradable realm demo #3147
Conversation
It's our initial approach to tackle to the upgradable realm feature. Please have a look. @moul @zivkovicmilos @Kouteki |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
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.
LGTM. will approve after this implementation integrated with other ones.
Moved to manfred_upgrade_patterns folder! |
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.
@@ -0,0 +1,5 @@ | |||
module gno.land/r/x/manfred_upgrade_patterns/upgrade_g/admin | |||
|
|||
require ( |
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.
Leftover?
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 mod tidy
|
||
var store admin.Store = nil | ||
|
||
func Init() { |
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.
Is this uppercase on purpose?
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.
It needs to be lowercase if this is supposed to be a realm constructor
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.
Not a constructor, meant to be called by the testing code as an emulated deployment
"gno.land/r/x/manfred_upgrade_patterns/upgrade_g/admin" | ||
) | ||
|
||
var store admin.Store = 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.
Why does store
need to be the global, and not an instance of logic
?
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.
The main question is why doesn't this store
instance live in the logic
instance?
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.
The logic
is simply a set of functions to be registered to the admin. It is a workaround with interface
s as they are not able to cross the realm boundary. Store, in this case, needs to be accessed by any functions in the realm, entrypoint or not. By having store as a top level variable it could have the similar coding style with having state variables.
|
||
var store admin.Store = nil | ||
|
||
func Init() { |
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 question, why uppercase?
|
||
func (s *store) GetCounter() uint64 { | ||
if s != currentStore { | ||
panic("Revoked store") |
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.
When is this possible?
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.
The user tries to call the public function in a old version logics without going through the admin interface. In this case, any outer-world call would fail because the prevRealm will return the logic's address, not the admin's. However, the old logics still holds the store reference, thus need to be deprecated when a new store struct is instantiated.
return currentStore | ||
} | ||
|
||
func Init() { |
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.
👀
std.TestSetRealm(std.NewUserRealm(alice)) | ||
std.TestSetOrigCaller(alice) // XXX: should not need this | ||
|
||
store.Init() |
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 understand now, you want to manually trigger the init :)
store.Init() | ||
|
||
v1.Init() | ||
urequire.Equal(t, admin.ReadCounter(), uint64(0)) |
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.
Nitpick, but the order of urequire.Equal
is expected
, then actual
(swap the last 2 values)
- Diamond pattern of admin <-> (multiple version of logics) <-> store | ||
- The admin provides persistent interface for the interaction for the other contracts and users | ||
- The store acts as the persistent storage independent from the migration | ||
= Logics could be replaced, registering the entrypoint functions as function variables to the admin and taking the store access(while revoking the previous one) |
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.
Nitpick: this is an =
, not a -
Run go mod tidy, fix README.md, fix testing, replied to comments |
The Also, please check why the CI is failing. |
Added upgradable realm demo.
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description