search

cartesi / rollups-contracts

Smart Contracts for Cartesi Rollups
https://cartesi.github.io/rollups-contracts/
Apache License 2.0
18 stars 38 forks source link

feat!: new `IConsensus` interface #172

Closed guidanoli closed 8 months ago

guidanoli commented 9 months ago

Closes #152 and #153 FYI @gligneul

ZzzzHui commented 9 months ago

I'm thinking whether we can get rid of the current AbstractConsensus.sol but replace it with the codes in EpochHashStorage.sol. So the inheritance chain becomes simpler: IConsensus -> EpochHashStorage(can be renamed as AbstractConsensus) -> Authority. What do you think?

guidanoli commented 9 months ago

I'm thinking whether we can get rid of the current AbstractConsensus.sol but replace it with the codes in EpochHashStorage.sol. So the inheritance chain becomes simpler: IConsensus -> EpochHashStorage(can be renamed as AbstractConsensus) -> Authority. What do you think?

We could also make Authority inherit from EpochHashStorage directly, but then it would have to implement getEpochHash (much like AbstractConsensus). So, I kept AbstractConsensus to avoid having to implement getEpochHash for every consensus that would like to use EpochHashStorage.

ZzzzHui commented 9 months ago

We could also make Authority inherit from EpochHashStorage directly, but then it would have to implement getEpochHash (much like AbstractConsensus)

It wouldn't. We rename the function _getEpochHash to getEpochHash and make it external.

guidanoli commented 8 months ago

We could also make Authority inherit from EpochHashStorage directly, but then it would have to implement getEpochHash (much like AbstractConsensus)

It wouldn't. We rename the function _getEpochHash to getEpochHash and make it external.

Ok, so you're suggesting to make EpochHashStorage an abstract contract that half-implements IConsensus? If we go in this direction, we could even rename it as AbstractConsensus. What do you think?

ZzzzHui commented 8 months ago

Yes. That's my suggestion :)

ZzzzHui commented 8 months ago

As a side note, I think maybe we can make the future changeset files a bit less verbose (like this one), or it should be better formatted. Otherwise, it can get hard to read.

guidanoli commented 8 months ago

As a side note, I think maybe we can make the future changeset files a bit less verbose (like this one), or it should be better formatted. Otherwise, it can get hard to read.

I agree that multi-line changeset files may look really cluttered on their own, but I don't think they look that bad when aggregated in a changelog. See the PR created by the CI for reference. I believe this is how this is going to be rendered in the final CHANGELOG as well.

ZzzzHui commented 8 months ago

I agree that multi-line changeset files may look really cluttered on their own, but I don't think they look that bad when aggregated in a changelog. See the https://github.com/cartesi/rollups-contracts/pull/147 created by the CI for reference. I believe this is how this is going to be rendered in the final CHANGELOG as well.

I do agree that they dont look bad at all when aggregated and look from a distance. But if reading from item to item, then it can be difficult because it's lengthy. But also I know it's gonna be helpful if users know what they are looking for and search for it, then the details would be very helpful that way.