`join_subsidy` and `exit_subsidy` interact incorrectly with pool after collection of subsidy

[maltekliemann]

The following tests result in unexpected results:

#[test]
fn join_subsidy_after_collection_fails() {
    ExtBuilder::default().build().execute_with(|| {
        assert_ok!(Swaps::create_pool(
            BOB,
            ASSETS.iter().cloned().collect(),
            ASSETS.last().unwrap().clone(),
            0,
            ScoringRule::RikiddoSigmoidFeeMarketEma,
            None,
            None,
            None,
        ));
        let who = ALICE;
        let pool_id = 0;
        let min_subsidy = <Runtime as crate::Config>::MinSubsidy::get();
        assert_ok!(Currencies::deposit(ASSET_D, &who, 2 * min_subsidy));
        assert_ok!(Swaps::pool_join_subsidy(Origin::signed(who), pool_id, min_subsidy));
        assert_eq!(Swaps::end_subsidy_phase(pool_id).unwrap().result, true);
        assert!(Swaps::pool_join_subsidy(Origin::signed(who), pool_id, min_subsidy).is_err());
    });
}

This test fails with the following error message:

---- tests::join_subsidy_after_collection_fails stdout ----
thread 'tests::join_subsidy_after_collection_fails' panicked at 'assertion failed: Swaps::pool_join_subsidy(Origin::signed(who), pool_id, min_subsidy).is_err()',

Instead, it should fail because providing subsidy after collection is illegal.

#[test]
fn exit_subsidy_after_collection_fails() {
    ExtBuilder::default().build().execute_with(|| {
        assert_ok!(Swaps::create_pool(
            BOB,
            ASSETS.iter().cloned().collect(),
            ASSETS.last().unwrap().clone(),
            0,
            ScoringRule::RikiddoSigmoidFeeMarketEma,
            None,
            None,
            None,
        ));
        let who = ALICE;
        let pool_id = 0;
        let min_subsidy = <Runtime as crate::Config>::MinSubsidy::get();
        assert_ok!(Currencies::deposit(ASSET_D, &who, 10 * min_subsidy));
        assert_ok!(Swaps::pool_join_subsidy(Origin::signed(who), pool_id, 10 * min_subsidy));
        assert_eq!(Swaps::end_subsidy_phase(pool_id).unwrap().result, true);
        assert_ok!(Swaps::pool_exit_subsidy(Origin::signed(who), pool_id, min_subsidy));
    });
}

This test fails, but for the wrong reason:

---- tests::exit_subsidy_after_collection_fails stdout ----
thread 'tests::exit_subsidy_after_collection_fails' panicked at 'Expected Ok(_). Got Err(
    Module(
        ModuleError {
            index: 5,
            error: [
                22,
                0,
                0,
                0,
            ],
            message: Some(
                "NoSubsidyProvided",
            ),
        },
    ),
)',

The NoSubsidyProvided is raised because end_subsidy_phase clears the vector of subsidy providers. Instead of raising this cryptic error, exit_subsidy should fail because the pool is in an incorrect state.

[sea212]

Good catch. It seems this can't be abused because it errors out (although for the wrong reason) when trying to remove subsidy from a pool after subsidy collecting state. I'll assign a low priority because nothing is in danger here.