refactor(prt-contracts): merge (non)leafTournament into Tournament

[stephenctw]

No description provided.

[GCdePaula]

Looking amazing 🤩

I've left a few comments. I'm still reviewing, but looks great!

[GCdePaula]

Could we remove (from the interface) instantiateTop and keep only instantiate? Like, it's ok to keep instantiateTop as a private function, for code organization, but I think it makes sense to remove it from the interface, and rely only on instantiate from the point of view of those outside.

[GCdePaula]

Would it make sense to always pass the "correct" values? Like, always pass the state transition smart contract, and always pass the tournament factory?

[guidanoli]

I agree. It makes the factory code simpler, with fewer branches.

[GCdePaula]

Could we always just use the Multilevel factory and delete the single level?

[guidanoli]

I was thinking about this as well.
Single-level tournaments are multi-level tournaments with levels = 1. :-)

[GCdePaula]

I see all these "getter" functions:

• nonRootTournamentArgs
• _stateTransition
• _tournamentFactory

Should we just always work with the CloneArguments? That is, anyone who needs to know any of the arguments, just access directly the CloneArguments and access the relevant fields.

Moreover, I'm thinking whether we should just unify all of them. This way, CloneArguments would become TournamentArguments, and TournamentArguments would be:

struct NestedDispute {
  Tree.Node contestedCommitmentOne;
  Machine.Hash contestedFinalStateOne;
  Tree.Node contestedCommitmentTwo;
  Machine.Hash contestedFinalStateTwo;
}

struct TournamentArguments {
  Commitment.Arguments commitmentArgs; // per tournament instance
  uint64 level; // per tournament instance
  uint64 levels; // per factory

  NestedDispute nestedDispute; // per tournament instance

  // Time constants, perhaps should be put
  // in a different struct for better organization
  Time.Instant startInstant; // per tournament instance
  Time.Duration allowance; // per tournament instance
  Time.Duration maxAllowance; // per factory
  Time.Duration matchEffort; // per factory


  IDataProvider provider; // per dispute
  IStateTransition stateTransition; // per factory
  IMultiLevelTournamentFactory tournamentFactory; // per factory
}

This is a part of the code I think we can improve more later. Perhaps some of these "per factory" values could be queried through the factory? That is, instead of storing the state transition, we could just ask the factory for it, since it's a constant for all tournaments created by that factory. But let's leave this for a later PR.

[GCdePaula]

I see some documentation comments were removed. Was that intentional?

[guidanoli]

+1 to a single view function for retrieving the clone arguments, which can be renamed to something less bound to ERC-1167 implementation details, like tournament arguments, which we will then need to either flatten out or rename.

[stephenctw]

I see some documentation comments were removed. Was that intentional?

Probably not, I'll double check.

[GCdePaula]

Let's create better errors. I think for this one it should be something like "NotALeafTournament".

[guidanoli]

I was thinking about the same thing.
Also, auxiliary functions like _isLeaf[Tournament] and _isRoot[Tournament] may help.
These functions may accept a TournamentArguments struct in memory as argument.

[GCdePaula]

Removing this comment was intentional?

[GCdePaula]

I think I'd consider removing the concretes folder. It only has Tournament.sol in it. Might be interesting to have something like:

src/
├─ tournament/
│  ├─ Tournament.sol
│  ├─ MultiLevelTournamentFactory.sol
│  ├─ IMultiLevelTournamentFactory.sol
│  ├─ libs/

[guidanoli]

Now that the abstracts directory is gone and there is only one concrete contract left, I agree this would be the next natural step! Plus, if we remove the single-level tournament factory, we can bring the multi-level tournament factory interface and concrete one level up, as you also suggested.

[stephenctw]

@guidanoli @GCdePaula Thanks guys for your awesome review! I've made the changes accodingly.