Skip to content
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

Add a protocol version to Store and World #1130

Closed
Tracked by #760
alvrs opened this issue Jul 10, 2023 · 8 comments · Fixed by #1511
Closed
Tracked by #760

Add a protocol version to Store and World #1130

alvrs opened this issue Jul 10, 2023 · 8 comments · Fixed by #1511
Assignees

Comments

@alvrs
Copy link
Member

alvrs commented Jul 10, 2023

  • Eg a view function returning the current version (since version doesn't change within an existing Store/World)
  • This allows for upgrades to the protocol while keeping support for previous versions
@alvrs alvrs added this to the Contracts stable milestone Jul 10, 2023
@alvrs alvrs mentioned this issue Jul 10, 2023
17 tasks
@holic
Copy link
Member

holic commented Jul 10, 2023

could/should add this to emit HelloWorld too, so it can be indexed alongside other events

@holic holic changed the title Add a ProtocolVersion to Store and World Add a protocol version to Store and World Aug 18, 2023
@holic
Copy link
Member

holic commented Aug 18, 2023

In the client, we should detect if the store/world protocol version matches the version we expect to parse and throw an error otherwise. I just spent a while debugging an issue with Sky Strife where they were using a different client from the contracts, so table data wasn't getting into the proper places due to version mismatch.

@johngrantuk
Copy link
Contributor

I thought this might be an interesting issue to take a look at contributing to but not really sure best approach.

Seems like currently a world emits a HelloWorld event on init and this is used by the SyncWorker to get the block number to start sync - is this what you mean by the client?

Some random thoughts:

Versions just stored as hardcorded values in World.sol and StoreRead.sol?

Emitting once:

  • e.g. during HelloWorld
  • Seems like should be enough rather than say, adding version to every event?

Adding a view function:

  • Easy/cheap to add
  • Can be read easily by anyone anytime (e.g. even on a block explorer)
  • Would likely add anyway?

Is a factory pattern interesting to consider?

  • Used a lot in DeFi for deploying certain canonical pools
  • More security. A pool from a known factory can be considered audited/safe/approved. There was an early issue with Zapper when Balancer launched - they listed a couple "Balancer" pools, failing to check whether they were deployed from the canonical factory - and people got rugged. Could maybe imagine a situation with worlds? At the moment anyone could deploy a malicious "World" that met the correct interface and make it emit and would be difficult to track client side which are real or not (not sure why they would at this stage but might be worth thinking about)
  • Also useful for configuring external logic. E.g. Balancer has logic in the factories to configure the pools. For instance, the pause window for each pool is set based on the time the factory was deployed. Even more importantly, some pools have auxiliary contracts or libraries that need to be "wired up" at deployment time, and again, we need to be sure that is done correctly and with our canonical code. For instance external maths.
  • Deploying from an approved Lattice factory supports off-chain monitoring. You can get a list of canonical pools just by monitoring events from the factory.

@holic
Copy link
Member

holic commented Aug 24, 2023

Some thoughts:

  • since store/world are immutable as far as the protocol they instantiate and use, I think it'd make the most sense to emit the protocol version on deploy rather than per event
  • we already want a deploy event to be able to more easily find deploys via a log filter, so it would make sense to bundle this and the version emission together
  • I think it makes sense to have view function for on-demand access makes, could just be a contract constant (unless we think there are use cases for overriding the version number in downstream child contracts)
  • I'm not super familiar with factories but it sounds useful! I'm not sure how that would factor in to our custom deploy tooling that wires everything together, or MUD being a sort of EVM chain-agnostic set of tools where the factory may not exist for your desired EVM chain cc @alvrs @dk1a

@alvrs
Copy link
Member Author

alvrs commented Aug 24, 2023

Agree with the thoughts on emitting the version as part of the deploy event and as a static function.

Re factories: I really like the idea! We could definitely move the whole vanilla world setup to a factory contract (deploying the World, installing the core module). If we combine this with something like CREATE2, the deploy script could check whether a World factory already exists for the chain the user wants to deploy to use it if it does (and if the user didn't specify a custom World contract in the mud config). A nice side effect is that the same core module can be installed in all Worlds deployed by the factory, since the module and the systems it installs are stateless.

@dk1a
Copy link
Contributor

dk1a commented Aug 24, 2023

Root systems can do anything and negate any security benefit of a canonical deployment. Also people may want to alter World, since it's a protocol/framework, not an application-specific contract.
But just for easier/cheaper deployment of default contracts factories might be nice

@holic
Copy link
Member

holic commented Aug 24, 2023

It might be nice to have deterministic deploys/addresses for systems, modules, etc. too so we can detect and avoid redeploying those too.

@johngrantuk
Copy link
Contributor

Just a note on people wanting to alter a World - I think it could be quite exciting if people do want to do that and it doesn't stop it. It might even make it easier to integrate into the overall system. At Balancer we've seen quite a few custom pool factories with different AMM behaviour developed independently by external teams and integrated into the overall system, mainly by indexing off different factory addresses on the Subgraph.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants