Review of the EthDIDAnchor contract

[coder5876]

Review of the EthDIDAnchor contract

Issues

Here are some issues I found with the EthDIDAnchor contract:

Use of both anchorHash and ipfsHash

It's not clear why we need both anchorHash and ipfsHash and what the relationship between them are. The implementation spec in the sidetree-core repo specifies the use of an "anchor file hash" that represents the hash of the file containing the batch of sidetree operations. It seems that we can just use one anchorFileHash in the transactions array.

Anyone can reset the mapping from anchor hash to ipfs hash

Anyone can call the function newAnchorHash with an existing _anchorHash and an arbitrary _ipfsHash. This overwrites the previous mapping in the anchors map. I'm not sure about the functionality of the two hashes (see above) but this is probably not the intended functionality.

Blocknumber should be recorded along with anchor file hash

The sidetree-core spec requires that the anchor file hash be recorded together with a transaction number (which is included here) as well as a transactionTime, which can be a block number. To remedy this another array blocknumber can be introduced where the blocknumber can be stored indexed by the transaction number.

Possible optimization: use only Events and not on-chain storage

Since we are not expecting that the data here should be readable by smart contracts on-chain we could log the data only as an Event (like what is done with AnchorHashCreated). This will avoid some unneccessary on-chain state but will still give strong guarantees about the data.

A downside of this optimization is that it is more cumbersome to read the data since it's not all available in the latest state database. The whole blockchain will need to be traversed sequentially.

Recommendations

Remove ipfsHash and the anchors mapping

This is not needed in order to be compliant with the sidetree-core spec. Only one hash is needed.

Add an array to keep track of blocknumbers

Add an array blocknumbers to map transaction numbers to block numbers in order to be able to get block numbers along with anchor file hashes.

For event-based optimization: use only transactionNumber and Events

If we want to do this optimization we may remove all on-chain data except transactionNumber and use an Event to store the following data: anchor file hash, transaction number, block number (block number may be reconstructed later so may can probably be omitted).

[OR13]

I think the best approach is to refactor the contract, implement the event based optimization, and then ensure that only one hash is stored. This should address all issues described in this ticket.

[coder5876]

@OR13 Ok that sounds good! I can do a first stab at refactoring, but I'd like input from @Therecanbeonlyone1969 (Andreas) on the points I brought up.

[OR13]

@christianlundkvist I took a stab at a simpler contract: https://github.com/decentralized-identity/sidetree-ethereum/blob/master/contracts/SimpleSidetreeAnchor.sol

I added some tests as well. feel free to open any issues regarding it.

I think we should drive towards a single anchor contract in the repo, interested to hear your thought on the topic. (should we remove the legacy contract now?)

[OR13]

Closing this for now, we will add support for more complicated contracts at a future date.