hicommonwealth / commonwealth

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

Chain-Events Community Semantics Origin Refactor #3881

Closed timolegros closed 1 year ago

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

Acceptance Criteria

timolegros commented 1 year ago

Blocked by #3228 -> contains quite a few changes to the structure of the subscriber therefore I will wait until it is merged before I make additional changes to the subscriber for this issue.

timolegros commented 1 year ago

After reviewing the changes that would be required CE app-side to query the new chain-events format it seems to me that simply redefining chain to be ChainNode.name and then optionally including a contract address in the event may be a better approach. This is because a separate contract_address column that can be indexed in the database is better for querying than having the name + contract_address combined in a single column. Thus on the backend the chain + address will be stored separately but we can of course still combine both into an origin for logging/display purposes if needed.

Since chain is updated to community by Ian CW side there should be no conflicts with CE. Most of the logic above holds it simply won't be renamed to origin and there will be an extra contract_address column. @jnaviask thoughts?

timolegros commented 1 year ago

3963 is related to this issue. If the chains with has_chain_events_listener = true change then #3963 may become a blocker.

timolegros commented 1 year ago

3991 should be merged before continuing here to avoid unnecessary work.

timolegros commented 1 year ago

4020 should also be completed and merged before continuing. The problem currently is that ChainEntities on the chain-events side are no longer related to any specific community while ChainEntityMeta inherently are since they are titles set by community members/authors on existing proposals. Since this is the case we have the option of either refactoring ChainEntityMeta to make them work within a community-agnostic CE or completely delete the table and the title setting functionality that it supports.

Given that #3991 will be merged soon and #3542 was already merged we now have no more use for the ChainEntityMeta table and we can completely remove it without any loss of functionality (considering that only Substrate supported title setting and the feature has been broken since the Listeners have been broken as well).

timolegros commented 1 year ago

To be clear, #3991 #4020 and #3806 need to merged/completed before this ticket will be unblocked.

timolegros commented 1 year ago

Still blocked by #3991 and #4020. Once those 2 are merged I can work on resolving merge conflicts and splitting the existing PR into 3 separate PRs to avoid app downtime during deployment (aka ensure backward compatibility between CE/CW and then PR to remove the backward compatibility).

timolegros commented 1 year ago

Also, this is not a 3-point ticket. This ticket has been ongoing with lots of testing + preliminary changes required. This is a 5-point ticket.

timolegros commented 1 year ago

If chain-events is removed, this issue can be closed.

timolegros commented 1 year ago

CE will be removed per #4696