Merge winners with participants

[sunce86]

Description

Extracted from https://github.com/cowprotocol/services/pull/2980/files

Merges participants and winners into a single vector, which is much easier to work with and forward around. Also reduces quite a few roundtrips when iterating over participants to determine winning and non-winning orders etc.

Winner selection logic is just moved into Runloop::competition function so that Participant could become a main struct we work with and try to even remove RawParticipant with refactors in the future, or at least make it local to competition only.

Changes

• Moved Participants into domain
• Participants now contain winner to determine if it's a winner and allowed to settle.

How to test

Existing e2e tests.

[MartinquaXD]

Would still be good to keep this log around IMO.

[sunce86]

Currently, if there are no valid solutions, we have a log for that.

If there is at least one valid solution, there is automatically at least one winner (and it's always a first one), so it can't happen that we have no winners in this case.

[MartinquaXD]

Not sure we gain much from this extra type. Instead we could initialize Participant with winner: false and have a second step that sets the value to true for the winners.

[sunce86]

In general, using the same struct to keep two or more different states of the object is an invitation for bugs to join the party (as we don't know which code now and in the future will use the object before second step).

If code duplication is issue here, I would rather have

#[derive(Clone)]
pub struct Participant {
    inner: RawParticipant,
    winner: bool,
}

impl Participant {
    pub fn driver(&self) -> &Arc<infra::Driver> {
        &self.inner.driver
    }

    pub fn solution(&self) -> &Solution {
        &self.inner.solution
    }

    pub fn is_winner(&self) -> bool {
        self.winner
    }
}

[squadgazzz]

I am also more inclined not to deal with mutating objects. In my exp, that often results in errors and more complex code.

[sunce86]

refactored according to example to remove code duplication

[MartinquaXD]

I don't really see how turning RawParticipant into a Participant (i.e. adding the winner field) should not be considered mutating objects. Before Dusan introduced the accessor methods both solutions were equally prone to misuse because you could just change the public winner field of the Participant anyway.
Is the concept of a RawParticipant really so important that we should have a domain object for it?

I really want to avoid introducing more and more types which seem the same but are not quite the same. Otherwise you'll have the mental overhead of having to know which subtle detail is different in this particular version of the type.
WDYT about this to reduce the ways mutation can lead to issues (again I feel like what we did with the 2 types should also be considered mutating). That way you can only mutate the structs once in a predictable manner that makes sense in the domain:

#[derive(Clone)]
pub struct Participant {
    solution: Solution,
    driver: Arc<infra::Driver>,
    winner: bool,
}

impl Participant {
    pub fn driver(&self) -> &Arc<infra::Driver> {
        &self.inner.driver
    }

    pub fn solution(&self) -> &Solution {
        &self.inner.solution
    }

    pub fn is_winner(&self) -> bool {
        self.winner
    }
    
    pub fn mark_as_winner(&mut self) {
        self.is_winner = true;
    }
}

[MartinquaXD]

If you are really adamant about type safety (i.e. having 2 types) I would at least make the names of the 2 structs more obvious: UnrankedParticipant and (Ranked)Participant.

[MartinquaXD]

We could even play around with the typestate pattern. Although this is admittedly quite a lot of boiler plate code to achieve a simple thing and should probably even have it's own file to limit the scope of the new state types:

struct Participant<State> {
    solution: Solution,
    driver: Arc<infra::Driver>,
    state: State,
}

struct Unranked;
struct Ranked {
    is_winner: bool
}

impl <T> Participant<T> {
    pub fn solution(&self) -> &Solution {
        &self.solution
    }

    pub fn driver(&self) -> &Arc<infra::Driver> {
        &self.driver
    }
}

impl Participant<Unranked> {
    pub fn new(solution: Solution, driver: Arc<infra::Driver>) -> Self {
        Self {
            solution,
            driver,
            state: Unranked
        }
    }

    pub fn rank(self, is_winner: bool) -> Participant<Ranked> {
        Participant::<Ranked> {
            state: Ranked {
                is_winner,
            },
            solution: self.solution,
            driver: self.driver
        }
    }
}

impl Participant<Ranked> {
    pub fn is_winner(&self) -> bool {
        self.state.is_winner
    }
}

[sunce86]

The biggest concern would be if we use non-initialized is_winner while assuming it's initialized. I think I would be fine with initializing in two steps, if the whole capturing of solve response and competition is done in a single step, for example, if we encapsulate that in a Competition::new() constructor or something.

But I found you typestate pattern most adequate for this use case so I took it. 👍

[m-lord-renkse]

Do we plan to add e2e tests for this change? 🤔

[m-lord-renkse]

Please, can you add unit test for these changes? it is not trivial code and small bug could be hidden 🙏

[sunce86]

We did not envision implementing unit tests for the runloop functionality (although I agree it could be useful for these small logically interesting parts).

For now I am happy if the changes do not break the current setup (running with 1 winner) which I regularly check on staging.

Once the whole feature is finished, I think having complete e2e test would be useful before activating the feature gradually on each network.

[m-lord-renkse]

LGTM