-
Notifications
You must be signed in to change notification settings - Fork 391
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: remove govdao dependency in r/gov/dao/bridge
#3523
Conversation
r/gov/dao/bridge
🛠 PR Checks SummaryAll Automated Checks passed. ✅ 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
|
Fixing tests... |
Lint has a bug, will write an issue on it. To be ignored for now. |
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, just two small comments. I would love to have some review from @zivkovicmilos too.
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.
Blocking until I review 🙏
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
Co-authored-by: Manfred Touron <[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.
Unblocking this PR, but left comments below, more as a historical reference. I see @moul has no objections to the way we use indirection and increase complexity for the bridge management, which signals to me this is not important, and I shouldn't care either 🙂
This is also not entirely true:
r/gov/dao/v2 imported r/sys/users to have access to user data.
The GovDAO doesn't need a dependency on users
for any critical function, but only to render the usernames. Let that sync in, we're trading this insane orchestration complexity for resolving an address to a username in a user-render function.
I spoke to @ajnavarro, we are dropping the bridge in future DAO implementations anyway for a more scalable method
Check the CI please 🙏 |
Waiting on #3597 to merge |
Description
Cherry-picked from: #3166
This PR removes the
r/gov/dao/v2
import from ther/gov/dao/bridge
realm. Previously, cyclic imports were easily created if a realm imported the bridge realm (to expose a executor constructor func), and GovDAO imported that realm. In my case:r/sys/users
importedr/gov/dao/bridge
to create executor constructorsr/gov/dao/bridge
importedr/gov/dao/v2
to have access to the implementationr/gov/dao/v2
importedr/sys/users
to have access to user data.This is fixed by modifying the
r/gov/dao/v2
contract to expose a safe-object, which in turn exposes all top-level functions as methods. Then, the bridge uses a one-time init package which will load v2 into the bridge in genesis, as per @moul's comment.