-
Notifications
You must be signed in to change notification settings - Fork 373
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: new govdao pattern with context #2380
Conversation
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
Signed-off-by: moul <[email protected]>
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 after two minor changes.
Co-authored-by: Antonio Navarro Perez <[email protected]>
Co-authored-by: Antonio Navarro Perez <[email protected]>
@ajnavarro, the changes have been applied. |
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.
Looks good 💯
I've left a few comments 🙏
I love that I now don't need a custom executor for calling Realm methods
## Description This PR improves the gno implementation of the `valopers` Realm. Waiting on #2380 to add a context-based Govdao pattern before merging. <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Co-authored-by: Manfred Touron <[email protected]>
## Description This PR improves the gno implementation of the `valopers` Realm. Waiting on gnolang#2380 to add a context-based Govdao pattern before merging. <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Co-authored-by: Manfred Touron <[email protected]>
This PR introduces a new pattern for arbitrary govdao proposals using Gno code using contexts. It involves wrapping the provided closure with a system that configures a `context.Context` with a field indicating that the execution occurs in the context of an `approvedByGovDao` proposal. This opens the door to a new ACL system, not based on `PrevRealm()`, but on a context-based "certification" system. I believe this pattern makes sense to exist; however, here are some drawbacks I can see: 1. `context.Context` is not yet needed (no goroutine support), and we may eventually never need this package depending on how we implement goroutines internally. (h/t @thehowl) 2. `context.Context` is used like an environment variable, which introduces implicitness—something we usually try to avoid to make Gno a very explicit language. Explicitness brings “simplicity” and “verifiability.” 3. The usual Go idiomatic way of using `context` is to pass it as the first argument. If this becomes more common, it will result in more contracts having exposed functions that are not callable by `maketx call` (though they can be called with `maketx run` or via an import). Depends on gnolang#2379 Related to gnolang#2386 --------- Signed-off-by: moul <[email protected]> Co-authored-by: Antonio Navarro Perez <[email protected]>
This PR introduces a new pattern for arbitrary govdao proposals using Gno code using contexts. It involves wrapping the provided closure with a system that configures a
context.Context
with a field indicating that the execution occurs in the context of anapprovedByGovDao
proposal. This opens the door to a new ACL system, not based onPrevRealm()
, but on a context-based "certification" system.I believe this pattern makes sense to exist; however, here are some drawbacks I can see:
context.Context
is not yet needed (no goroutine support), and we may eventually never need this package depending on how we implement goroutines internally. (h/t @thehowl)context.Context
is used like an environment variable, which introduces implicitness—something we usually try to avoid to make Gno a very explicit language. Explicitness brings “simplicity” and “verifiability.”context
is to pass it as the first argument. If this becomes more common, it will result in more contracts having exposed functions that are not callable bymaketx call
(though they can be called withmaketx run
or via an import).Depends on #2379
Related to #2386