Redux: Requirements for smart contract content directory

[bedeho]

Redux: Requirements for smart contract content directory

Problem

Our current content directory has a bespoke, complex and arbitrary write access model that has multiple independent strands in order to model ownership, relationship constraints and ration resource utilisation. It is a very inflexible model in terms of what functionality it support, and increasing this scope requires compounding the already disorganised write access model.

Proposal

This proposal for requirements for a smart contract based content directory rests on the analysis here

#1518

and the assumption that the, yet to be conducted, benchmarking analysis finds that it is practical and safe to accept the need for migrations in rare cases.

Note

This is not a specification, it is a draft for a set of requirements for a specification.

Prior Approach

There was an exploratory attempt at sketching out an implementation for a smart contract based content directory here

#368

The outcome was evaluated to be quite positive, however more recent lessons suggest that even this evaluation was too pessimistic, specifically

• MUCH smaller migrations: Almost no data needs to be stored, now that we have a query node, at worst there are some hash commitments, but even those are of unknown utility, and there there are genuine business logic values that need to be read back later. All large fields, like human readable text blobs, URLs, structured metadata, is not stored. This means state to migrate is orders of magnitude smaller than before.
• Long running migrations: Migrations can run over more than a single block when state mutexed properly. This was always the case, it was just not clear in my mind. This means large migrations are possible, as long locking of system is acceptable.
• Less frequent migrations: Most changes to the write logic require no migration at all, only deploying new contracts at worst, because most changes will not change how they read or write from storage, or they will read objects in a backwards compatible way.

Requirements

EVM

We are using the EVM module because it has the best supporting tooling and the broadest developer adoption at this time. One important limitation is worth keeping in mind when using the EVM FRAME module, which actually also applies for the Contract FRAME module. This limitation is that it is not possible to make any call, synchronous or asynchronous, from the smart contract environment and into the rest of the runtime. In the same vein it is not possible to read or write to external storage. However, it is possible to make the following synchronous calls into the smart contract environment from the runtime:

• Call: Call a method on a contract.
• Deploy: Deploy a new contract.
• Deposit: Deposit funds from a balances account to a smart contract account.
• Withdraw: Withdraw funds from a smart contract account into a balances account.

This impacts how to integrate external parts of our runtime with the content directory, such as the content directory working group, the proposal system, and so on. The key take away here is that these integrations cannot depend on being able to read external runtime state, as is typical for example when we wish to authenticate someone as holding a role in a working group, or being a given member. Inverse integrations, such as proposals updating the state in the content directory, works just fine, you just need some contract for the proposal system, and only the proposal system, to be able to invoke.

We want to retain the feature that any user can submit new smart contracts in the EVM, this will be essential for allowing new ownership abstractions not built in to the system.

Flexible Proposal System Integration

At all times there must be a hard coded account, only updatable with a runtime upgrade, referred to as the council smart contract account. This account is the equivalent of the funds and contract caller that the council has the right to use directly.

There should be at least the following types of proposals:

1. Smart contract budget adjustment: This can either be a new proposal, or an augmentation of the existing budget adjustment proposal, where tokens are transferred between the council smart contract account and the council budget, in either direction.
2. Constrained contract invocation: Invoke Call on any contract as council smart contract account. Gas is still paid, and (1) can be used to finance it.
3. Constrained contract deployment: Invoke Deploy for any contract as council smart contract account. Gas is still paid, and (1) can be used to finance it.
4. Unconstrained EVM Invocation: Any EVM invocation is allowed with any parameters, e.g. as any calling contract. This is a very risky call, as it allows the council to behave as a sort of SUDO, hence the proposal parameters should probably be quite restrictive, similar to a general runtime upgrade. There is no risk of blowing up the runtime, like in runtime upgrades, however funds can be confiscated from any user. (Is this worth it?)

It's worth nothing that 2 above actually allows for extreme flexibility in how the content directory can be updated in the future. For example, what it means to be a curator, curator group and so on, can be entirely altered.

Metadata

Metadata refers to that data which is write only, that is the values are not relevant to any state transition. For content, metadata is typically things like titles, descriptions and so on. The majority of the fields found in runtime schemas shipped for Babylon Network constitute such metadata. Recall that such metadata is not to be stored in state. The old content directory attempted to have schema based validation of the metadata itself, however this has limited value. Instead, the metadata for any entity in the content directory should just follow the semantics of a raw blob based dictionary. This means that when initialising metadata for some entity, such an initial dictionary is provided, and then there is a subsequent ability to set, or unset mappings in this dictionary for a given entity. Here are some hypothetical metadata mutators for Channel entities:

• Set initial dictionary: makeChannel(..., initial_metadata: BTreeMap<Vec<u8>, Vec<u8>>)
• Adds new, or overwrites existing, mappings in dictionary: setChannelMetadata(update_mappings: BTreeMap<Vec<u8>, Vec<u8>>)
• Clears mappings from dictionary: unsetChannelMetadata(drop_keys: BTreeSet<Vec<u8>>)

The benefit of treating the metadata as a dictionary, rather than a singular blob, is that it allows for more space efficient updating of parts of the metadata, as compared to uploading a new blob for every change. Do of course keep in mind, none of the metadata is held in state, the dictionary mutations only occur virtually.

Lastly, both curators, and any owner, should be able to update metadata.

Roles

There are three roles in the Joystream runtime which make their way into the content directory

• Lead: The lead of the corresponding working group, here called the curator lead or just lead.
• Curator: A worker in the corresponding working group, here simply called a curator. The lead is also a curator.
• Member: A member in the membership registry.

There is also the concept of a curator group, which is simply an identifiable group with a dynamic membership set of curators, managed by the lead. Such groups are meant to represent an intermediary for managing channels belonging to the platform itself. Each of these concepts are explained in the following sections.

Lead

A lead can directly act as a curator and as a member of any curator group.
A lead can also be deactivated or activated by the council through a proposal, when deactivated, the lead cannot perform any lead action.
The working group lead can only act as a lead as long as they have are not unstaking.

Curator

A curator can act as a member of a curator group, the significance of which is explained later, and otherwise perform curator activities like deactivating/activating channels or videos, or deleting videos.
A working group worker can only act as a curator as long as they have are not unstaking.
A curator can only be part of up to a hard coded number of working groups which is sufficiently low that it is considered safe and economical to iterate and remove the curator from all such groups in the event that the curator the working group.

Be aware that since the EVM cannot dial out to the working group to authenticate callers as workers, it must be the case that there is some EVM state accumulated about what memberships exist, and what their controller accounts are at any given time. This will requires introducing hooks in the working group module that call into the EVM to build synch this state.

Curator Group

A curator group is created by the lead, and the lead can manage the membership of each curator in each group. A group can be deleted only when no curators are members and no channels are owned by the group. Each curator has independent permissions associated with membership in a given group, describing which among the following are available in a group owned channel

• Publish: Publish new videos.
• UpdateVideo: Update video metadata.
• Delete: Delete videos.
• UpdateChannel: Update channel metadata.

It follows that channels owned by curator groups do not follow the normal permission model, where the curators normally can do lots of things (see Channels). Only the lead can create a group channel and manage what groups maintain what channel.

Members

A member can create a new channel and own it as a member.

Be aware that since the EVM cannot dial out to the membership module to authenticate callers as members, it must be the case that there is some EVM state accumulated about what memberships exist, and what their controller accounts are at any given time. This will requires introducing hooks in the membership module that call into the EVM to build synch this state.

Channels

Channels satisfy the following requirements.

• Each channel is identified by a unique immutable identifier. Any metadata referencing a channel should use its identifier.
• Any contract can create a channel, and thus becomes the owner. It follows that channels are owned by raw accounts. This abstraction can then be used to layer on top controls through DAOs, membership or working group roles.
• There is a global cap on the total number channels that can exist. This cap can be raised via a proposal from the council.
• Each channel has an upper bound on the number videos that can exist in it at any given time. All channels share the same default bound, and this default is updated
• A channel has non-stored metadata with dictionary semantics.
• A channel owner can update the ownership of a channel to any other account.
• A channel owner can publish a video under the corresponding account.
• A channel owner can remove a video under the corresponding account.
• A channel owner can remove a channel provided it has no videos.
• A curator can remove a video under any channel, with a provided reason.
• A curator can de-active and reactive a video under any channel, with a provided reason for each. Compliant UIs & infrastructure should not offer should not display videos that are de-activated.
• A curator can de-active and reactive a video under any channel, with a provide reason for each. Deactivation implies
  • it is not possible to publish new videos under the channel
  • it is not possible to update channel metadata
  • compliant UIs & infrastructure should not offer the channel or any video in the channel.
• A curator can extend or restrict the quota of a channel, within global limits. These limits can only be updated by a proposal.
• Changes to the business logic of a channel which do not depend on changing the storage model of a channel in backwards incompatible way should be possible without any migration of storage state.

Videos

Videos satisfy the following requirements.

• Each video is identified by a unique immutable identifier. Any metadata referencing a video should use its identifier.
• There is a global cap on the total number video that can exist. This cap can be raised via a proposal from the council.
• A video has non-stored metadata with dictionary semantics.
• Changes to the business logic of a video which do not depend on changing the storage model of a channel in backwards incompatible way should be possible without any migration of storage state.

Specification

As stated, this description is more a description of requirements, and a more detailed specification is needed to move forward. This specification must describe

• contract types required
• contract instantiations of different types.
• how contract instances interact.
• changes to the proposal system
• changes to the membership module
• changes to the working group module

[bedeho]

I suggest we do this in two iterations, first iteration avoids any need for upgradability, as described by this bullet point

Changes to the business logic of a _ which do not depend on changing the storage model of a channel in backwards incompatible way should be possible without any migration of storage state.

Then we can do a second iteration where we consider whether its worth itnroducing some kind of upgradability. The major approaches seem to be separate data and delegatecall proxying, but there seems to be substantial tradeoffs is safety, maintainability of this

• https://blog.trailofbits.com/2018/09/05/contract-upgrade-anti-patterns/
• https://consensys.github.io/smart-contract-best-practices/software_engineering/#example-2-use-a-delegatecall-to-forward-data-and-calls
• https://blog.trailofbits.com/2018/10/29/how-contract-migration-works/

Storage separation, but without eternal storage, seems like the most acceptable tradeoff in terms of complexity and benefit.

[bedeho]

This seems like a very useful resource

https://github.com/paritytech/frontier

[Lezek123]

Initial design draft

This just an initial design draft of content directory smart contracts describing:

• what kind of contracts we need
• what is the role of each of those contracts and what kind of storage / logic it contains
• how the runtime interacts with those contracts
• how those contracts interact with each other
• how upgrades / migrations can be performed (considering 3 example scenarios)

Modifiers

Inside the Solidity pseudocode provided under Contracts I assumed the existance of some basic function modifiers that would be used to check permissions:

• _onlyRuntime - msg.sender must be an address hardcoded in the runtime (in this case we assume it's not a council address, but an address that is used only by the runtime to update "bridge" contracts state, ie. mapping of member controller addresses to member ids)
• _onlyCouncil - msg.sender must be a council address hardcoded in the runtime (methods using this modifier can be executed through proposals)
• _onlyLead - msg.sender must be an address associated with current content working group lead and the lead must also not be deactivated (which can be done by the council). We will be able to verify this using the "bridge" contract.
• _onlyCurator - msg.sender must be an address associated with an active curator (which also includes lead)
• _onlyMember - msg.sender must be an address associated with an active member
• _onlyPrimary - msg.sender must be the primary contract (the one that "owns" given contract)

Contracts

This sections describes some basic contract instances required, along with some pseudocode that briefly describes the storage and methods of each of those contracts,

Members.sol - membership module bridge

This would be a very simple "bridge" contract, updated by runtime membership module.
It will provide the map of addresses (controller account ids converted to evm-compatible address format) to member ids, which can be used by other contracts to check if msg.sender is a member controller.

    mapping (address => uint64) controllerAddressToMemberId
    setMemberAddress(_memberId, _address) _onlyRuntime

ContentWorkingGroup.sol - content working group module bridge

A bridge to content working group runtime module, providing information about current lead address and worker (curator) addresses (role keys converted to evm-compatible addresses). It also gives council the ability to disable/enable the lead (disabled lead can no longer perform any lead-only operations in the evm environment)

    mapping (address => uint64) curatorIdByAddress

    address currentLeadAddress
    boolean isLeadActive

    setCuratorAddress(_curatorId, _address) _onlyRuntime

    setLeadAddress(_address) _onlyRuntime

    setLeadStatus(_status boolean) _onlyCouncil

    getActiveLeadAddress()
        - returns currentLeadAddress, unless lead is not active, in which case returns address(0)
        - this can be used by _onlyLead check 

CuratorGroupStorage.sol (owned by ContentDirectoryLogic.sol)

A storage contract (managed by the logic contract) containing all data related to curator groups (migration is only be needed in case the curator group storage representation needs to be change)

    // Group permissions w.r.t. group channels
    // Instead of GroupPermissions struct we may want to have
    // a more generic representation of permissions here (ie. mapping (uint8 => boolean) permissionsFlags)
    // to prevent the need for migrations in case we want to add other flags.
    // In case the flags are just a (uint8 => boolean) mapping, we can assign
    // some specific meaning to a flag by its "id" inside ContentDirectoryLogic contract
    struct GroupPermissions {
        updateChannel: boolean
        publishVideo: boolean
        updateVideo: boolean
        deleteVideo: boolean
    }

    struct Group {
        permissions: GroupPermissions
        isExisting: boolean
    }

    // curatorId => groupId => boolean double-map representing curator membership in group
    mapping (uint64 => mapping (uint16 => boolean)) isCuratorInGroup
    mapping (uint16 => uint32) curatorCountInGroup
    mapping (uint16 => Group) groupById
    uint32 nextGroupId = 1

    addGroup(_permissions) _onlyPrimary
        - update the map and nextGroupId
    addCuratorToGroup(_groupId, _curatorId) _onlyPrimary
        - update isCuratorInGroup double map
        - ++curatorCountInGroup[_groupId]
    removeCuratorFromGroup(_groupId, _curatorId) _onlyPrimary
        - update isCuratorInGroup double map
        - --curatorCountInGroup[_groupId]
    // Is there a value in removing a group that has no curators and no channels anymore?
    removeGroup(_id) _onlyPrimary
        - delete group from map (will set isExisting to false)

ChannelStorage.sol (owned by ContentDirectoryLogic.sol)

A storage contract (managed by the logic contract) containing all data related to channels (migration is only be needed in case channels' storage representation needs change)

    // Instead of ChannelOwnership we could have a more generic representation of ownership here
    // (ie. ownershipType: uint8, ownerId: uint256)
    // to prevent the need for migrations in case the ownership model is changed/expanded
    enum OwnershipType { CuratorGroup, Member, Address }
    struct ChannelOwnership {
        ownershipType: OwnershipType
        address: address // in case it's owned by Address (ie. a DAO)
        memberId: uint64 // in case it's owned by a Member (member controller account addresses can then change)
        groupId: uint16  // in case it's owned by a Curator Group (group members/permissions can then change)
    }

    struct Channel {
        ownership: ChannelOwnership
        active: boolean
        isExisting: boolean
    }

    map(uint64 => Channel) channelById
    map(uint16 => uint32) channelCountByGroup // Needed in removeCuratorGroup check
    // We could also have channelCountByMemeber and channelCountByAddress for more extendability
    // (or in case ownerId is just a generic u256 - channelIdByOwner)
    map(uint64 => uint64) videoLimitByChannelId
    uint64 nextChannelId

    addChannel(_ownership, _metadata) _onlyPrimary
        - update the map and nextChannelId
        - update channelCountByGroup/channelCountByOwner if applies
    updateOwnership(_id, _ownership) _onlyPrimary
        - update in map (channel.ownership)
    updateStatus(_id, _status) _onlyPrimary
        - update in map (channel.active)
    setChannelVideoLimit(_id, _limit) _onlyPrimary
        - update videoLimitByChannelId
    removeChannel(_id) _onlyPrimary
        - delete channel from map (will set isExisting to false)
        - update channelCountByGroup/channelCountByOwner if applies

VideoStorage.sol (owned by ContentDirectoryLogic.sol)

A storage contract (managed by the logic contract) containing all data related to videos (migration is only be needed in case videos' storage representation needs change)

    struct Video {
        channelId: uint64
        active: boolean
        isExisting: boolean
    }

    map(uint64 => Video) videoById
    map(uint64 => uint64) videoCountByChannel // We need it to enforce maxVideosPerChannel and during removeChannel check
    uint64 nextVideoId

    addVideo(_channelId) _onlyPrimary
        - update the map and nextVideoId
        - ++videoCountByChannel[_channelId]
    updateStatus(_id, _status) _onlyPrimary
        - update entry in map (video.active)
    removeVideo(_id) _onlyPrimary
        - delete video from map (will set isExisting to false)
        - --videoCountByChannel[videoById[_id].channelId]

ContentDirectoryLogic.sol

This is the main logic contract that the frontend will interact with. In terms of the actual code it can be split into a few abstract contracts for better readibility (so it doesn't need to be just a one huge Contract { }), but for the purpose of this very simplified representation we can assume that this is just a one, huge contract.

We should be able to deploy a new version (instance) of this contract without the need for any data migration (the details of this process are further described in the Migration Scenarios section).

    // Storage contracts references
    ChannelStorage channelS
    VideoStorage videoS
    CuratorGroupStorage curationGroupS

    // References to "bridge-contracts"
    Memberships memberships
    ContentWorkingGroup contentWorkingGroup

    // Limits (they can me "migrated" just be setting a default value here, so no need to keep them in a separate storage)
    uint64 channelInstancesLimit
    uint32 videosPerChannelLimitDefault
    uint32 videosPerChannelLimitMax

    // Perhaps the actual higher-level definitions related to GroupPermissions / ChannelOwnership
    // should actually live here (see: ChannelStorage / CuratorGroupStorage)

    // Event definitions (ie. ChannelCreated etc.)

    // Limit setters that can be called via proposal module
    setChannelInstancesLimit(_limit) _onlyCouncil
    setVideosPerChannelLimitDefault(_limit) _onlyCouncil
    setVideosPerChannelLimitMax(_limit) _onlyCouncil

    // A flag that will allow disabling any content directory interactions (effectively locking current storage state)
    // which will be useful for data migrations
    isLocked: boolean
    modifier _onlyUnlocked
        - a modifier that checks if isLocked == false (should be used by all methods that affect the storage)

    setLocked(_locked) _onlyCouncil
        - can be used by the council to lock current content directory state (disabling the use of all methods that
          affect the storage

    constructor
        - will take addresses of storage/bridge contracts as arguments and set them in storage

    // This is a method that will faciliate the migration process in case new logic/storage contracts are deployed
    migrate(
        _newLogic address,
        _newVideosStorage address,
        _newChannelsStorage address,
        _newGroupsStorage address
    ) _onlyCouncil _onlyUnlocked
        - will transfer ownership of existing storage contracts (in case no new storage contract address is provided)
        - if `_newXXXXXStorage` address is provided - it will destroy the old storage contract and set the new logic contract
          as owner of the provided (new) storage contract (this is further detailed in Migration Scenarios section)
        - emit an event containing addresses of new constract instances
        - selfdestruct to make sure this contract is not used anymore


    // CHANNELS

    createChannel(_ownership ChannelOwnership, _metadata: string[2][]) _onlyUnlocked
        - verify if msg.sender can create a channel with provided ownership:
            - only ContentWorkingGroup.lead can create CuratorGroup owned channel
            - only member can create Member owned channel
            - anyone can create Address owned channel
        - nextChannelId <= channelInstancesLimit
        - call channelS.addChannel
        - emit an event

    updateChannelMetadata(_channelId, _metadata: string[2][]) _onlyUnlocked
        - channel exists
        - channel is active
        - check if msg.sender has permissions (depending on ChannelOwnership and possibly GroupPermissions)
        - emit an event (this will be the only source of information that this data has been updated)
          emitting and event with ~1kB of metadata costs ~100k gas, which seems acceptable

    updateChannelOwnership(_channelId, _ownership ChannelOwnership) _onlyUnlocked
        - channel exists
        - check if msg.sender has permissions (depending on ChannelOwnership and possibly GroupPermissions)
        - only lead can update ownership to CuratorGroup
        - call channelS.updateOwnership
        - emit an event

    updateChannelVideoLimit(_channelId, _limit) _onlyUnlocked
        - verify channel exists
        - verify msg.sender is a curator
        - in case channel is owned by group - verify msg.sender is also an active lead
        - _limit < videosPerChannelLimitMax
        - call channelS.setChannelVideoLimit
        - emit an event

    removeChannel(_channelId) _onlyUnlocked
        - channel exists
        - check if msg.sender has permissions (depending on ChannelOwnership and possibly GroupPermissions)
        - videoS.videoCountByChannel[_channelId] === 0
        - call channelS.removeChannel
        - emit an event

    deactivateChannel(_id, _reason) _onlyUnlocked
        - verify channel exists
        - verify channel is currently active
        - verify msg.sender is a curator
        - in case channel is owned by group - verify msg.sender is also an active lead
        - call channelS.updateStatus
        - emit an event

    activateChannel(_id) _onlyUnlocked
        - verify channel exists
        - verify channel is currently inactive
        - verify msg.sender is a curator
        - in case channel is owned by group - verify msg.sender is also an active lead
        - call channelS.updateStatus
        - emit an event

    // CURATOR GROUPS

    addCuratorToGroup(_curatorId, _groupId) _onlyLead _onlyUnlocked
        - curator exists
        - group exists
        - Curator is not already in group
        - call curatorGroupS.addCuratorToGroup
        - emit an event

    removeCuratorFromGroup(_curatorId, _groupId) _onlyLead _onlyUnlocked
        - curator exists
        - group exists
        - Curator is currently in group
        - call curatorStatusS.removeCuratorFromGroup
        - emit an event

    // Is there a need for removeCuratorFromAllGroups?
    // If a curator leaves / gets terminated it wouldn't be able to act as a group member, because the
    // address => curator connection will no longer exist
    removeCuratorFromAllGroups(_curatorId) _onlyLead _onlyUnlocked
        - curator exists
        - iterate over curator groups and call curatorStatusS.removeCuratorFromGroup
          (or implement this in storage, since that should lower the gas cost, but this can still easily
          get very costly in case there are a lot of groups)
        - emit an event

    // Is there a need for removeCuratorGroup?
    // A group with no channels and members can't perform any action anyway unless "revived" by the lead
    removeCuratorGroup(_id) _onlyLead _onlyUnlocked
        - group exists
        - channelS.channelCountByGroup[_id] === 0
        - curationGroupS.curatorCountInGroup[_groupId] === 0
        - call curationGroupS.removeGroup
        - emit an event

    // VIDEOS

    addVideo(_channelId, _metadata) _onlyUnlocked
        - channel exists
        - verify msg.sender has permissions (depending on ChannelOwnership and possibly GroupPermissions)
        - videoS.videoCountByChannel[_channelId] < channelS.videoLimitByChannel[_channelId] || videosPerChannelLimitDefault
        - call videoS.addVideo
        - emit an event

    updateVideoMetadata(_id, _metadata) _onlyUnlocked
        - verify video exists
        - video is active?
        - verify msg.sender has permissions (depending on ChannelOwnership (videoById[_id].channelId) and possibly GroupPermissions)
         - emit an event (this will be the only source of information that this data has been updated)

    removeVideo(_id) _onlyUnlocked
        - verify video exists
        - verify msg.sender has permissions (depending on ChannelOwnership (videoById[_id].channelId) and possibly GroupPermissions)
        - call videoS.removeVideo
        - emit an event

    removeVideoAsCurator(_id. _reason) _onlyUnlocked
        - verify video exists
        - verify msg.sender is a curator
        - in case video's channel is owned by group - verify curator group membership and permissions
        - call videoS.removeVideo
        - emit an event

    deactivateVideo(_id, _reason) _onlyUnlocked
        - verify video exists
        - verify video is currently active
        - verify msg.sender is a curator
        - in case video's channel is owned by group - verify curator group membership and permissions
        - call videoS.updateStatus
        - emit an event

    activateVideo(_id) _onlyUnlocked
        - verify video exists
        - verify video is currently inactive
        - verify msg.sender is a curator
        - in case video's channel is owned by group - verify curator group membership and permissions
        - emit an event

UpdatedStorage.sol

This is an abstract contract that can be extended by some new storage contract that we want to migrate the data into (ie. contract VideoStorage is UpdatedStorage). It can faciliate the migration process described in Migration Scenarios.

    isInitialized boolean = false
    address initializator
    address owner // owner/primary address that will be set during ContentDirectoryLogic.migrate
    address oldLogicContract // Address of old (current) ContentDirectoryLogic.sol

    modifier _initializationOnly
        - require that msg.sender == initializator
        - require that isInitialized == false

    modifier _migrationOnly
        - require that msg.sender == oldLogicContract
        - require that the owner is not set

    constructor(_oldLogicContract)
        - set initializator and oldLogicContract addresses (initializator can just the msg.sender)

    // I'm not sure it'll be possible to actually have an abstract function like this in Solidity
    // (since in reality it will take very varying types of arguments depending on the storage contract)
    // but here it serves as a way to signal that the contract that extends this one should implement some method like this
    abstract batchInsert() _initializationOnly

    setInitializationFinished() _initializationOnly
        - sets the isInitialized to true (can only be called once)

    setOwner _migrationOnly
        - sets the first owner (new logic contract). Will only get called once during migration process
          (described further in Migration Scenarios section)

Migration scenarios:

Deploy new logic (ContentDirectoryLogic.sol):

In this case we just want to update some logic inside ContentDirectoryLogic.sol. This is a pretty straghtforward 2-step process that requires no data migration:

1. Deploy a new instance of logic contract (providing the addresses of current storage contracts to constuctor)
2. Call old logic contract's migrate method with new logic contract address as first argument (it will pass ownership of storage contracts to the new logic contract)

Both of those can be done by council via proposals system, but only the second one really needs to by done via proposal (since anyone can deploy a contract that can be then verified and accepted by the community as "the new logic contract")

Deploy new logic and updated storage (ie. ContentDirectoryLogic.sol + VideoStorage.sol)

1. Lock current content directory storage state with ContentDirectoryLogic.setLocked(true) (this can be done by the council through proposal at any point before collecting the data that will be migrated)
2. Deploy a new instance of storage contract (ie. VideoStorage.sol) that extends UpdatedStorage.sol and implements a batchInsert method
3. Create migration script that will call batchInsert() (in most cases it will take multiple blocks to fully migrate the data)
4. Run a script that will migrate the data using batchInsert() methods
5. Call setInitializationFinished to lock the storage state (at this stage the new storage contract will contain all the initial data, yet it will unusable, because its owner is not yet set)
6. Deploy a new instance of logic contract (with addresses of current Channel and CurationGroup storage contracts and the new VideoStorage contract)
7. Call old logic contract's migrate method providing the new addresses of logic and VideoStorage contracts
   • It will pass ownership of ChannelStorage.sol and CuratorGroupStorage.sol and destroy/deactivate old VideoStorage.sol
   • It will set the new logic contract as owner of the new VideoStorage (making it finally usable)

Steps 2-6 can be performed by any actor without any special privelages (this can, but doesn't need to be, a council member).
After step 6 we would have a deployed, ready-to-migrate ContentDirectoryLogic instance with the new VideoStorage instance containing all the migrated, initial data (it cannot be in any way modified at this point). Because everthing is locked at this point, the council is able to verify the validity of the new contract instances and safely execute the final step (7) through proposal system (steps 6 and 7 are very similar to steps 1-2 in the previous scenario).

Deploy new logic and brand new storage (ie. ContentDirectoryLogic.sol + MusicStorage.sol)

The benefit of having separate storage contract instances for each data type (ie. Video, Channel etc.), besides the fact that we don't need to migrate all the data if we only change the storage representation of a single one of them (ie. Video), is that we can easily introduce completely new data types (ie. Music) without the need for any migration at all (in this case we assume that we don't need to change Channel representation, but in reality this may be the case)

1. Deploy a brand new storage contract (ie. MusicStorage.sol)
2. Deploy a new, updated instance of ContentDirectoryLogic contract (with reference to the new storage and all the necessary changes in logic)
3. Call old logic contract's migrate method with new logic contract address as first argument (it will pass ownership of all current storage contracts to the new logic contract)

Just like in the first example, the council only needs to execute the last step. One thing to note is that here we also need to make sure that the migrate function of the new ContentDirectoryLogic contract implements the neccessary logic of transferring MusicStorage to the new owner (just like it does for other storage contracts)

Query node integration

The query node should be able to handle events emitted by the contracts, the most straighforward way to achieve it would be probably to rely on substrate evm pallet Log event (we would need some functions to decode the hex data, but this can probably be done with the help of some existing web3.js methods)

In this design proposal, the contract that emits all the events that the query node will need is ContentDirectoryLogic. The query node only needs to have the initial address of this contract hardcoded, but since this address may change over time (after upgrades/migrations), it should be able to replace the address it currently recognizes as ContentDirectoryLogic address based on event emitted by ContentDirectoryLogic.migrate (this will be a rather simple-to-handle event like: Migrated(newAddress))

Possible improvements

We might want to have a separate contract that handles the migration logic (ie. transferring stroage contract ownerships etc.), acts as a provider of current instance addresses for each contract type and perhaps stores some additional data related to migrations.
This could be the main contract that the council would interact when executing upgrades/migrations.
This may be a cleaner way to handle upgrades/migrations, but it's also possible that it will just introduce additional, unnecessary complexity (I didn't give it all that much thought yet, since I'm not conviced that it's necessary, but this can be perhaps reconsidered at a later stage).

[bedeho]

Overall

Phenomenal work, and extremly fast turnaround, very excited to see this get implemented.

General

• Why do we need to distinguish different caller origins on the antive runtime side? Whether its the council (i.e. proposal system), membership module or something else, the EVM does not ever need to distinguish the call, because we control all of them as rutnime authors, and we know what can invoke what EVM entry points.

• Can we some how sidestep the isExisting approach for checking presence of mapping?

• The lack of algebraic types is just a disaster, e.g. for ChannelOwnership, there is no way around this?

membership module bridge

• Does it not make more sense to use the member id as the key, as it is immutable, but also as there is no uniqueness constraint over controller accounts.
• I think you also need to hook into controller account being updated for a member that has already been added.
• How should we deal with broken invariant: e.g. the runtime wants to update the controller account of a member that has never been added.

content working group module bridge

• Same points.
• You are mixing representations for how to show if a lead is active. In storage you have isLeadActive, but then you also in getActiveLeadAddress overload the address type with a special value. Lets avoid this dual approach, either we do one or the other.

CuratorGroupStorage

• can't curatorCountInGroup value just live in Group, and then the map can get dropped, and we have one less state invariant? It is only needed there in order to block removal of groups having dangling curator members.

ChannelStorage

• shouldnt channelCountByGroup value just live in Group, same reason. Its only there to block removal of a group having dangling channels.
• shoudlnt videoLimitByChannelId value just live in Channel, same reason.

VideoStorage

• shouldnt videoCountByChannel value just live in Channel, same reason. Its only there to block removal of a channel having dangling videos.

ContentDirectoryLogic

• Should the dependency on storage contract be through interfaces, rather than full implementatino contracts?

[Lezek123]

Why do we need to distinguish different caller origins on the antive runtime side? Whether its the council (i.e. proposal system), membership module or something else, the EVM does not ever need to distinguish the call, because we control all of them as rutnime authors, and we know what can invoke what EVM entry points.

If we use the same address for methods that are called through proposals and the methods that are called by the runtime to, for example, update the bridge contracts' state (ie. Memberships), then we cannot prevent the bridge contracts from beeing broken through a proposal (in the current model _onlyCouncil cannot call _onlyRuntime methods).

I suppose it would make no sense to break them intentionally, but it's just an additional security measure (alternatively we can just have _onlyRuntime and not distinguish between that and _onlyCouncil)

Can we some how sidestep the isExisting approach for checking presence of mapping?

We could maybe use OpenZepplin's EnumerableMap (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/EnumerableMap.sol), but it seems to have a pretty big limitation:

(...) This means that we can only create new EnumerableMaps for types that fit in bytes32.

Perhaps we could try implementaing our own library to sidestep that.

The lack of algebraic types is just a disaster, e.g. for ChannelOwnership, there is no way around this?

In ChannelStorage I was actually leaning toward having a more generic representation where, for example, ownership type could be just uint8 and owner id could be uint256 (which can store address, hash, number etc., depending on our needs), then we could have a library that would parse this data to/from higher-level representation. This way we could update the logic and, for example, introduce new ownership types without the need for any data migration (we could just start using ownershipType of value 4, which would mean Curator and would also imply that ownerId is actually a curatorId and needs to be converted from uint256 to uint64)

Does it not make more sense to use the member id as the key, as it is immutable, but also as there is no uniqueness constraint over controller accounts.

I was thinking in context of easily retrieving memberId of given msg.sender, but I didn't take into account that one address can control multiple members. In that case it looks like we need to take _memberId as argument in each call that needs a specific member context, so we can then check if msg.sender == addressByMemberId(_memberId).

I think you also need to hook into controller account being updated for a member that has already been added.

I was assuming the runtime would execute all the necessary evm calls to Membership.setMemberAddress() in order to always keep the map up to date.

How should we deal with broken invariant: e.g. the runtime wants to update the controller account of a member that has never been added.

In the current example Membership.setMemberAddress() is just the simplest setter, it works exactly the same way both in case of new membership beeing created and existing member address beeing updated.

Perhaps we can have separate methods for that, implement additional checks and emit an event from the contract in cases like the one mentioned, so that any inconsistencies could be more easily spotted.

You are mixing representations for how to show if a lead is active. In storage you have isLeadActive, but then you also in getActiveLeadAddress overload the address type with a special value. Lets avoid this dual approach, either we do one or the other.

I agree, onlyLead should probably just verify both varaibles and the getActiveLeadAddress() "shortcut" is not really valueable. We also cannot just use a single variable with address(0) meaning that the lead is deactivated / not set, because then every call to reactivate the lead would need to include the lead address and that would be very error prone and could conflict with the calls coming directly from the runtime's working group module (resulting in inconsistencies between evm environment and the runtime).

can't curatorCountInGroup value just live in Group
shoudlnt videoLimitByChannelId value just live in Channel, same reason.

Good points, it's possible to simplify it this way

shouldnt channelCountByGroup value just live in Group
shouldnt videoCountByChannel value just live in Channel

Those are a bit more tricky because, for example, it's addChannel / removeChannel actions that tirgger changes in channelCountByGroup, so in this case the ChannelStorage would need to call GroupStorage (if we want this number to be updated in Group). I aimed for the storages to be as independent as possible (so no storage "owns" other storage), to avoid introducing additional complexity (that would be especially troublesome during migrations)

[Lezek123]

As discussed with @bedeho, we should allow updating the metadata of both disabled channels and videos, since the reason to disable a channel/video may be related to its metadata, in which case we would want the owner to fix the issue while the channel/video is still deactivated, so it can be then verified and reenabled by the curator.

This imples that:

• Video deactivation currently has no effects inside the smart contracts environment (only the UI is affected)
• The only effect of channel deactivation in the smart contract environemnt is preventing the owner from adding new videos to the channel (all other actions are permitted)

[bedeho]

Was tried.