Content directory - initial smart contracts

[Lezek123]

This PR includes the initial code of new content directory Solidity smart contracts.

The implementation is based on #1520, the initial design was described in #1520 (comment)

I also icluded a few simple tests that can be run against a Ganache node:

cd smart-contracts
yarn
yarn start
yarn test

Of course there is also a possibility to run any other Truffle command, ie.:

yarn truffle deploy

Currently we rely on Ganache to run the tests, but once the evm pallet is enabled in the runtime it would be possible to run some test against the actual Joystream node (this was further described here: #1681)

Covered:

• very simple bridge contracts allowing only the "runtime" (which in test scenarios is just a random address generated by the Ganache node) to set members / content working group data
• storage contracts for channels, videos and curator groups
• ContentDirectory.sol contract currently containing all the logic related to managing channels, curator groups and videos. It probably needs to be split into separate contracts / libraries as described in the Things to consider section below
• a few example tests to run against a Ganache node

Not yet covered:

• storage contracts interfaces
• upgradability / migration / initialization logic (there is a migrate method in ContentDirectory.sol but it's not yet fully functional)
• full testing scenarios

Things to consider:

• the methods that can be executed both by a curator and member (ie. updateChannelMetadata) have kind of awkward "overloads", since we additionaly need _curatorId as context in case the method is executed by curator. There are a few alternatives, but each of them has some drawbacks:
  • make _curatorId arg part of the original methods like updateChannelMetadata, removeVideo - in this case this value could just be ignored on the smart-contract side if it's not needed (ie. sender has owner access to channel), but it would have to still be provided on the frontend (where it can be just set to 0 if it's not needed). This saves us some code in the smart contract, but seems like a bit less "cleaner" approach.
  • add separate, reverse curatorIdsByAddress map in ContentWorkingGroupBridge - we could then derive the curator context from the msg.sender address, but it would require a loop and could potentially cost a lot of gas in case the address controlls many curators
  • add an argument similar to current content directory Actor to all content directory contract methods - this wouldn't look very good in Solidity due to lack of enums that can hold additional values depending on the variant. It also adds complexity on the frontend.
  • add Actor argument to evm.call extrinsic in the runtime and use special msg.sender address format for members/curators - this would also allow us to get rid of bridge-contracts, but at the expense of not beeing able to verify whether a curator / member id is valid from insde of the evm environement (we would only know that the current msg.sender is valid curator/member)
• reducing the size of ContentDirectory.sol - currently it's a very big contract that costs a lot of gas to deploy (exceeds the default gas limit) and requires compiler optimalization specifically for deployment in order to prevent other issues. It needs to be split into libraries / other contracts or optimalized in some other way.
• non-metadata fields of channels/videos - we may want to store some of the Channel / Video fields like handle or contentId to validate whether they are unique, prevent them from beeing updated etc. There is also an open question w.r.t. how to handle the metadata that is clearly invalid (and in which case, if any, it should be handled on the smart contracts side)

[bedeho]

Here is my response to your notes

Not yet covered

Don't do any of this except tests, the upgradebility can wait, and it requires a deeper review of alternatives.

Things to consider

1. I think each approach had pretty nasty costs, what about just separating out different methods for each caller? so FooAsOwner and FooAsCurator, it's not as nice as what we can do in Rust, but it seems better than any of the alternatives listed.
2. reducing the size of ContentDirectory.sol: My impression is that, while there are many methods, it's all relatively simple stuff, and the state is very simple. If that is the case, we can just keep going forward and refactor later to find the best way to organize things, often this is only clear at a later stage.
3. non-metadata fields: Yes, if they genuinely are not just normal metadat fields, then they have to be lifted out. Unique handle would be a proper example. Its not obvious that contentid has to be one of those, and also the way we encode content source will change in the future when we update things on the storage side, so I would keep it as is. As a general rule, lets agree on a case by case basis before anything is lifted out.

[ondratra]

A very nice kickoff on the whole thing, Leszek! Some notes:

1. We should use int256 and uint256 instead of a smaller number types by default and use smaller types only when it brings some concrete advantage. In our use case, (only?) Rust/Solidity interface and events should share a smaller number type. In Ethereum, 256bit number is default Word and downscaling it costs extra gas - storing uint8 costs more than storing uint256, the same goes for calculations. This is one of the (possibly significant) causes of too expensive contract deployment (~6871595 gas).

2. In general, it might be a good idea to split the smart contract into multiple contract definitions that each focus on individual aspects. It will make code inspection easier as the contract gets bigger. In our case, ContentDirectory contract could be split into one part focusing on council-controlled mechanics (onlyCouncil functions), second focusing on channels, third on videos, etc.; and then joined via contract ContentDirectory is ContentDirectoryCouncil, ContentDirectoryChannels, ... {. In a sense, this is mentioned in the original PR description.

3. Right now, events are emitted from the main ContentDirectory contract, and storage is saved in a changeable contract(s). One small con of this is the lack of event emits in the storage - we can't simply walkthrough/audit storage operations using Ethereum logs. It might also create confusion/problems when a new storage contract is set - events may not necessarily correlate to storage contents. Another thing we need to keep in mind when doing the next iteration on upgradability is that when new storage is introduced, it needs to somehow include the old storage content in its inner workings because copying the whole storage content to the new contract will be expensive. We should definitely create two versions of storage contracts and a test for an actual migration and afterward querying data before the release.

[ondratra]

This function is never used.

[Lezek123]

Nicely spotted! It was supposed to be used instead of ownership.isValid in createChannel/updateChannelOwnership. I'll fix it and add a related test case

[ondratra]

Is there a reason why to use a 1-based index here? It seems to me that a regular 0-based index is used in the content directory pallet.

This topic is relevant to nextChannelId, nextGroupId, and nextVideoId.

[Lezek123]

I'm used to 1-based index when it comes to "entity ids", since that's usually how ids are assigned in MySQL/PostgresSQL databases etc. (and since query node uses PostgresSQL database it seems to make sense).

Also it looks like currently we're using 1-based index in the runtime too:

joystream/node/src/chain_spec/content_config.rs

Line 238 in 4865e9b

next_entity_id: 1,

[ondratra]

Not a problem on its own, but we should be careful when using enums in multiple contracts as there are non-intuitive issues while upgrading contracts. See https://hackernoon.com/beware-the-solidity-enums-9v1qa31b2 .

Either we should limit their existence to only one contract or write a WARNING commentary next to each such enum to prevent this kind of error as it might not be revealed by regular tests (as stated in the article).

[ondratra]

What string[2][] actually represents? I'm a little bit puzzled on what is the motivation behind the [2][] part (even number of characters?). Could we replace it with struct so we can use a more expressive data type name like Metadata through the code?

[Lezek123]

It's a dynamic-size array of arrays containing exactly 2 strings. Initially it was meant to represent cusomizable metadata with dynamic key-value pairs, ie.:

[
  [ 'title', 'Some video title' ],
  [ 'description', 'Some video description' ]
]

But since this is not very flexible, in #1816 I explored an alternative possiblity to replace it with just a string containing json metadata representation (I think that's probably the best way to solve this if the contracts don't need to care about metadata validity and structure)

[ondratra]

Since _ownership.isMember() != _ownership.isCuratorGroup() is always true, we can return right after require check

if (_ownership.isMember()) {
    require(...);
    return;
}

[ondratra]

Is it ok that the check for .isAddress() is not here with the rest of the ownership checks?

[Lezek123]

In case of membership/group we can additionaly check if it exists, but with address I don't think we need any additional checks (except maybe disallowing zero-address)

[ondratra]

Instead of type-specific implementations of SafeMath (SafeMath16, SafeMath32, ...) not originating from OpenZeppelin, we should use SafeMath(256bit) in combination with OpenZeppelin's SafeCast. Maintaining multiple math libraries that are not supported by OpenZeppelin would create for us an unnecessary maintenance burden and security weak point, especially when (eventually) updating the Solidity version. See OpenZeppelin/openzeppelin-contracts#1625 (comment)

[Lezek123]

I stumbled upon this link when trying to figure out the best solution, but it seemed to add a lot of burden to perform this casting after every operation. Since this is related to the general u256 comment, I'll address it further below.

[ondratra]

Just a code style note for your consideration. As the Object Calisthenics recommends, it is good to evade writing else and else if and instead of such branching style use process of elimination style instead.

if (_ownership.isAddress()) {
   return _address == _ownership.asAddress();
}

// easily commentable
if (_ownership.isMember()) {
  return membershipBridge.isMemberController(_address, _ownership.asMember());
}
...

[Lezek123]

we should use int256 and uint256 instead of a smaller number types by default and use smaller types only when it brings some concrete advantage

After thinking it through I agree.

At the beginning I could see a few benefits of using smaller types, ie.:

• Smaller storage cost when used inside Structs
• Possibility to easily limit some numbers to predictable size
• The types are more-or-less compatible with the current Rust runtime implementation

But considering the major drawbacks on the other side, ie. the need to either use custom SafeMath or "manually" cast integers with SafeCast after each operation + the additional costs when used outside Stucts (which as you said may be one of the causes of very high deployment cost) , probably it's better to just enforce min/max value when necessary and avoid using smaller ints/uints