hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
67 stars 41 forks source link

🪣 Fix Chain/Community Semantics #2535

Open CowMuon opened 1 year ago

CowMuon commented 1 year ago

We need to clean up the semantics around how we handle (define) chains & communities. We've been discussing this for a bit, it's about time to get going on fixing it.

ianrowan commented 1 year ago

*Throwing research into comments here as I go

Database root table + branches

Image

The following tables would need a definite refactor per column names for chain id. And on further research there are many more tables referencing chain w/o a foreign key relationship(will documents these in next comments)

ianrowan commented 1 year ago

All tables referencing Chain

Image

ianrowan commented 1 year ago

Current Scope of DB refactor look like it would include

  1. Renaming of the following tables
    • ChainCategory -> CommunityCategory
    • ChainEntityMeta -> CommunityEventsMeta ??
    • Chains -> Communities
  2. Update all foreign key relationships of Chain.id
  3. Update all usage of chain/chain_id to community_id and add a fkey constraint where not currently

App Scope

timolegros commented 1 year ago

After talking with Ian we agreed it makes sense to split this ticket into 2 PRs.

  1. Purely semantic changes PR for renaming tables, foreign keys, etc.
  2. Functional PR that introduces origin for events/entities which defines where the event/entity came from without referencing a community (Chains). This will affect proposal links, event generation/storage, and ChainEntityMeta.

As mentioned in-call, I'll follow up with another comment describing some of the changes that are required chain-events side (mostly relating to PR 2).

timolegros commented 1 year ago

The Problem

Currently ALL events and entities are directly linked to a chain from the Chains model (being renamed to Communities). This is restrictive since it means an event cannot be consumed by more than 1 community. If we want a system that is truly a general indexer we need to be able to determine the origin of an event or entity without referencing any CW-specific data.

The Solution

Thus we should introduce a new data point called origin.

Directory structure refactor

Chain-Events Library Refactor

Chain-Events Services Refactor

ChainSubscriber

CE Database

jnaviask commented 1 year ago

@timolegros really great breakdown -- please break it out into a separate, linked ticket for chain event specific discussion.

@ianrowan couple comments:

ianrowan commented 1 year ago

@jnaviask

  1. Got it will take a look at that and check what implications on the app side will be included, but looks like it should be minimal to do that as a quick separate task
  2. This makes sense, especially with the new way we'll be thinking about CE given Tim's comment and changes to polling that are already in progress. Only the chain column will need to be update in CEMeta
  3. /4. I think the immediate de-risk here would be to find a way to initially support the DB migration with minimal app change, ie aliasing columns to old chain names very temporarily. In this case CE related work could be unblocked by the migration and changes to app could be made fresh off of the migration as an immediate separate task.

As far as automation, I'll keep looking into this. I've done some initial research here, and havent found an all encompassing solution(strategies I've seen so far just update models and really direct usage of them, but not nested etc) The de-risk on this side would be to continue to support aliases || actual column names until we can remove those guardrails

jnaviask commented 1 year ago

How exactly would aliasing work? Would it be at the database or the server level? If we can figure out a clean scope of work here, then we should create a ticket and get moving.

ianrowan commented 1 year ago

@jnaviask aliasing would happen at the app level in the models the way I'm thinking about it. Basically the exported attributes and the table definitions keys remain the same(as used across the app), but the options allow us to specify what column a given key points to.

So for example, somewhere using Addresses.chain_id could still use that. but the model(in the app) would resolve that to community_id

And from there we could chip away at removing aliases/replacing with correct naming.

The Chains table itself would be the only table name to change, so I think at a minimum we would refactor usage of Chains but alias all fkeys in other models where they are used

jnaviask commented 1 year ago

I really like this solution @ianrowan. Please put up a ticket for it, linked to this ticket (which will be converted to bucket), and then we can start executing.

ianrowan commented 1 year ago

@jnaviask created issue #3779

timolegros commented 3 months ago

@ForestMars @jnaviask I'd like to transfer this ticket to someone on the PENG team as the only remaining variable semantic changes are on the client i.e. app.chain and someone from PENG should lead that change given they will have a better understanding of when/how to do this. That part of the refactor was delayed because PENG had anticipated potentially removing the app state objecting but months have passed and we have not progressed much in that direction.

I previously noted that this should be transferred to PENG on May 2: https://github.com/hicommonwealth/commonwealth/issues/5335#issuecomment-2089316120.