feat: handle prewitness deposits

[msgmaxim]

Pull Request

Closes: PRO-1145

Checklist

Please conduct a thorough self-review before opening the PR.

• I am confident that the code works.
• I have updated documentation where appropriate.

Summary

Pre-witnessing

When a deposit is pre-witnessed, we check if it can be boosted. Specifically, a deposit is boosted if:

• Channel's boost fee is >0.
• Amount is >= MinimumDeposit (otherwise we'd be boosting a deposit that would later be rejected).
• There is enough "available" funds in boost pools with fee <= channel's boost fee.
• There isn't already a boosted (and not finalised) deposit on the channel.

If the deposit is boosted:

• each pool involved in the boost records the amounts owed to each booster (their contribution + fee), which would become available when the deposit is finalised.
• ingress fee is deducted from the amount provided by boosters (note this is different from non-boosted deposits, where we deduct the fee on finalisation)
• perform channel action with the amount = deposit_amount - ingress_fee - boost fee.
• DepositBoosted event is emitted

Full witnessing/finalisation

When a deposit is finalised, we check if the channel is boosted (for the matching amount), and if so, we now credit the booster pool instead of executing the channel action.

• The funds are distributed according to booster_contributions records
• DepositFinalised event is emitted (renamed from DepositReceived) with action: DepositAction::BoostersCredited.

New extrinsic: add_boost_funds

• Debits the booster and adds these funds to the boost pool (along with a record linking them to the booster)
• If booster adds funds while being in the process of withdrawing, we add the funds and change their status to “not withdrawing” (for simplicity)

New extrinsic: stop_boosting

• Removes linked to the booster available funds from the pool and puts them into free balances.
• If a portion of booster's funds are awaiting finalisation, we mark the booster as "withdrawing" so that we know to return the funds into booster's free balances rather than back to the pool.

Other Changes

• Changed MockBalance to actually track the amounts (needed for tests)
• MockAssetConverter now correctly returns remaining amount after deducting fees
• Refactor: extracted perform_channel_action as a function, so that it can be called from two places: on boosting, and on finalisation if not boosted (as before).
• DepositChannelDetails now contains an extra filed, BoostStatus, which indicates whether there is a pending boost on some deposit into the channel (and which pools have been used).

Questions:

• What fields should we add to the DepositBoosted event? I have added boost_fee which is the total boost fee, but do we need to break that down by pools? Along with the amounts contributed from each pool?
• Which fields should be included inBoostersCredited ?

---

Migrations

• Create all boost pools
• Update DepositChannelLookup entries by adding boost_status of NotBoosted

Benchmarks

1. Added a benchmark for on_lost_deposit, which depends on the number of boosters (who participated in the boost). Updated existing on_idle hook to additionally use the weight of on_lost_deposit for any lost deposit.

(For extrinsics, I wasn't sure if it is possible/appropriate to read storage items in the pallet::weight annotation to return an accurate weight. For some benchmarks, I ended up picking some reasonable values instead, leaning towards overestimating the weight.)

2. Added a benchmark for add_boost_funds (this one was straightforward)

3. Added a benchmark for stop_boosting (here the weight depends on the number of pending boosts, I chose 100, which is probably an overestimate, at least at this stage)

4. Added benchmarks for boosting and finalising deposits. Because both of them are called from process_deposits, I chose to use finalise_deposit here because it significantly "heavier". In fact, I wonder if we could fine tune the parameters a bit (I chose 100 boosters, with 30% of which are withdrawing resulting in a DB read/write for each to update their balance). Or perhaps there is a way to make this benchmark use real data at execution time?

[msgmaxim]

Do we need these migrations still? I added boost status to get the code to compile, but it would be more correct for this to have a separate (e.g. DepositChannelDetails_v3) structs since this migration only updates from v2 to v3. Easier to just delete these of course if we don't need them. Similarly, I see that boost_fee was added here too, but boost_fee was only introduced in v5.

[kylezs]

Yeah I think so. The idea with not merging migrations like this is a) it's easier to see the correctness of a migration if it's not mixed with another and b) it allows us to easily slice the commit history. And say we're releasing at some earlier commit. If the migration was merged into this, then we'd have to rewrite it in that case.

[msgmaxim]

I discussed this with @dandanlen yesterday and agreed that we can already remove these migrations (since we aready have a branch for 1.3), which makes things easy. It is something we should keep in mind in the future though (Dan suggested we should more proactively delete already applied migrations to avoid this).

[msgmaxim]

@dandanlen are you going to remove stale migrations separately, or do you want this to be done in this PR (I don't think it would be a big deal to merge as is and remove them later IMO).

[dandanlen]

Yep I'll deal with it.

[AlastairHolmes]

Can you add and use a fn all() as above.

[msgmaxim]

I can do that, but because I have generic TargetChainAsset types (unlike the any::Asset you are referring to), I would have to add a new trait with fn all() to set it as a bound and then use it where I'm currently using strum::IntoEnumIterator, which doesn't seem much different from using strum::IntoEnumIterator directly... Is there another reason you prefer fn all() and do you still think it is worth adding?

[kylezs]

Still reviewing, but here're some initial comments

[kylezs]

Just general comment on these tests, I think they're good candidates for fuzzing. We could add ranges of values in some tests rather than using one set of specific values per test too.

[msgmaxim]

Are you talking about just adding a some tests with randomized values, and running them in some loop, or actually integrating some kind of third-party fuzzing framework/library? To clearify, is this a requirement before merging this PR, or just a nice to have that we can do later?

[kylezs]

I think some tests with randomized values is going to get us 80% of the way there - made some references to such specific situations in some other comments. I would say there are some cases we should do in this PR, like the boost_pool logic, to ensure rounding etc. is correct, but other cases can wait.

[kylezs]

feel like we should be able to write a macro, or generic function so that we can have each test for each chain without repetition, ik we don't do this many other places but I think it's a nice thing to start doing more often

[kylezs]

Is this right? We only have 1000, but we need to provide 1010 in boost funds? The boosted amount can only be a maximum of the amount in the pool? How can we provide more than we have?

[msgmaxim]

Maybe the terminology could be improved here, but I (indirectly) use "provide" to mean amount deducted + fee. "Boosted amount" is probably more accurate, but not sure how that would work with the function name (I don't like boost_funds since that would sound more like the entire amount is to be boosted by that pool IMO).

[kylezs]

maybe returning a struct that can have named params would help to clarify it here?

[msgmaxim]

Update: added migrations and benchmarks. Added corresponding sections to the PRs description. @kylezs

[codecov]

Codecov Report

Attention: Patch coverage is 81.77966% with 215 lines in your changes are missing coverage. Please review.

Project coverage is 72%. Comparing base (8feefa8) to head (fc2240d).

Files	Patch %	Lines
...ate-chain/pallets/cf-ingress-egress/src/weights.rs	15%	132 Missing ⚠️
state-chain/pallets/cf-ingress-egress/src/lib.rs	83%	20 Missing and 18 partials ⚠️
...f-ingress-egress/src/migrations/add_boost_pools.rs	68%	25 Missing ⚠️
...-chain/pallets/cf-ingress-egress/src/boost_pool.rs	95%	7 Missing and 4 partials ⚠️
.../pallets/cf-ingress-egress/src/boost_pool/tests.rs	99%	2 Missing and 1 partial ⚠️
state-chain/runtime/src/chainflip.rs	0%	2 Missing ⚠️
state-chain/traits/src/mocks/lp_balance.rs	94%	0 Missing and 2 partials ⚠️
...hain/pallets/cf-ingress-egress/src/benchmarking.rs	99%	0 Missing and 1 partial ⚠️
state-chain/traits/src/mocks/asset_converter.rs	0%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #4698    +/-   ##
======================================
  Coverage     72%     72%            
======================================
  Files        417     420     +3     
  Lines      69531   70481   +950     
  Branches   69531   70481   +950     
======================================
+ Hits       50245   50985   +740     
- Misses     16879   17063   +184     
- Partials    2407    2433    +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kylezs]

just "deposit_finalised"? We use it on process_deposits, and it's not necessarily the case we have a boost

[msgmaxim]

Are you asking why this doesn't also contain code for processing deposit normally? Or just about renaming it? I use boost_finalised as benchmark for deposit_finalised because it is (by far) the more expensive code path. (Is there a better technique for benchmarking code with branches?) I use boost_finalised as the name to better reflect the code that's actually being benchmarked.

[kylezs]

was just suggesting renaming it. usually the benchmarks are named after their extrinsic, I can see this case is a little unique though, we're benchmarking the boost pathway through this extrinsic - so guess I'm indifferent will leave it to you

[kylezs]

I think the main thing left to investigate here is the prewitness deposit ids and assess if we actually need them now. We could simplify a couple of things by removing them, so worth considering.

[kylezs]

Chatted to Max yesterday, we can look at factoring out the prewitness ids in another PR. Would await @dandanlen 's review before merging.

Nice one 👍

[dandanlen]

LGTM, great work.

I've added some suggestions. The only one I feel little strongly about is the comment about the type arg on ScaledAmount.Everything else is subject to discussion.

Up to you @kylezs if we address any of this before merging.

[dandanlen]

For future reference - Substrate provides DefaultNoBound (amongst others like CloneNoBound, DebugNoBound etc.) to help with this.

[dandanlen]

I think the C: Chain constraint is overkill here.

We could just use ScaledAmount<A: AtLeast32BitUnsigned>. This will also work nicer with scale metadata as there's less indirection.

[msgmaxim]

The reason I chose Chain is for extra type safety, i.e. to ensure that if I convert from EthAmount, then we can only convert back to EthAmount. Admittedly, the code would probably be hard to misuse even without this trait bound, but you could make this argument about other uses of C::ChainAmount. I'm not sure about the implication for the scale metadata though, so if you think it is important, I can remove C: Chain. (Not sure if AtLeast32BitUnsigned would be needed here though, what would A be constraining in that case?).

[dandanlen]

This is mostly repeated above (in on_finalised_deposit), there's some scope for encapsulation here. Maybe BTreeMap<Account, Balance> could be abstracted out into its own BoostPoolFragment type or something with some well-defined primitives like split/withdraw/combine. This might make it easier to reason about the code too.

[msgmaxim]

Tbh, I'm not sure what's being suggested here. Not sure how BoostPoolFragment wtih, say, splitting would come into play. Are you talking about amounts rather than pending_withdrawals?

The two methods here do look quite similar, but we are also talking about 10 lines of code or so, where the body of the loop is mostly different. I could maybe create a function remove_prewitnessed_deposit which would return (somewhat unexpectedly) a bool indicating whether the booster is "withdrawing", which we could use to do different things in on_finalised_deposit and on_lost_deposit, but that doesn't look like a good/clear interface. It seems to me that in this case the (cognitive) cost of indirection isn't worth marginal benefit from deduplicating a few lines of code. We can probably make equally good arguments either way TBH. Maybe I'm misunderstanding your suggestion?

[dandanlen]

Some doc comments would be nice, in particular to explain what the return amounts are.

[dandanlen]

Some of my comments got lost inside VS Code - adding them here for posterity.

---

I think it would help if we break up the BoostPool struct into two items: The BoostPool (with the fee rate and amounts are available for boosts) and an individual Boost (with the pending boosts and withdrawals).

Then when we boost we have a method like fn BoostPool::boost(&mut self) -> Boost and then Boost::return(self, &mut BoostPool)/Boost::socialize_losses(self) or something. But the basic idea is that you could use a BoostPool to get a Boost, and then a Boost either evaporates, or is returned to the pool. This seems better than including everything in the same struct and every function taking &mut self despite only using a subset of self.

I would also suggest including the fee tiers in the BoostPool instead of having a separate pool per fee tier. Seems like it might make testing simpler, and makes it easier to iterate etc. Not sure about this.

---

I'm not really a fan of on_* naming for things that are not hooks. By hooks I mean some trait with methods that we expect to be called at well-defined lifecylce events. But in this case, there is no indirection, and only one implementation, so I feel like a name that describes what happens is more useful.

[msgmaxim]

I think it would help if we break up the BoostPool struct into two items: The BoostPool (with the fee rate and amounts are available for boosts) and an individual Boost (with the pending boosts and withdrawals).

I gave it a quick try, and I don't think this is a clear improvement. Most BoostPool's methods need to access at least two, but usually three out of the three fields:

add_funds:
	- amounts, pending withdrawals
boost:
	- amounts, boosts
finalised:
	- amounts, boosts, pending withdrawals
lost:
	- boosts, pending withdrawals
stop:
	- amounts, boosts, pending withdrawals	

So yeah, we can put boosts and pending_withdrawals together and separate them from amounts, and then on lost deposits we will only be accessing one struct, but that seems like the only advantage, and it is rather minor. This is the only case where we "every function takes &mut self despite only using a subset of self". In all other cases we will have to access both structs anyway. Note, for example, that we need pending_withdrawals even when adding amounts since we want to undo "stop_boosting" in order to avoid extra complexity (we currently assume that a booster who stopped has 0 amounts available in the pool).

Seems like there will be some downsides to splitting the struct too (apart from the time it would take). Storing them as separate storage items would mean more storage api quieries (unless we are talking about storing them together in yet another struct). For tests it also seems more convenient to have a single struct.

[msgmaxim]

I would also suggest including the fee tiers in the BoostPool instead of having a separate pool per fee tier. Seems like it might make testing simpler, and makes it easier to iterate etc. Not sure about this.

I'm also not sure about this. Having a single boost tier as a struct boundary seems to make sense (even conceptually we think of them as separate entities). If we combine them, we'd have to update all fields to be BTreeMaps where values are the current fields (some of which are already BTreeMaps of BTreeMaps)... this would make the code unwieldy and more error prone.

Maybe you are talking about keeping the existing BoostPool and wrapping it another struct combining all tiers? We could do it I suppose, but to me it doesn't seems like it'd make a big difference, as we'd simply be moving the same code from one place to another. Maybe we'd be able to test the iteration more easily, but it not that complex and I think we already a pretty good coverage by the tests in tests/boost.rs.

[msgmaxim]

I'm not really a fan of on_* naming for things that are not hooks.

I see your point regarding on_* methods and I generally agree. I used these names as a placeholder initially, but better names never came to me. At the same time, while something like socialize_losses (as you used above) would better communicate what the code actually does compared to on_lost_deposit, it would make it less clear under which circumstances the function is called. We could further iterate and arrive at something like `socialize_losses_from_lost_deposit", but there is benefit to being concise too (and one could think that this is only part of the logic for handling lost deposits.) I would say it's actually not unusual to abstract away from the implementation when naming (in this case the implementation detail being that the losses are "socialized").

How about handle_lost_deposits? I know "handle" is quite generic too, but at least handle_* isn't normally used for "hooks", which seems to be your main concern. Same for "handle_finalized_deposit". I'm not a big fan of "handle_" names myself, but they "get the job done" when a better name isn't available, and are better than using a name that isn't quite accurate (as "return_finalised_deposit" would be IMO, since we normally talk about "boosting" a deposit rather than "giving" it, and it wouldn't say anything about boost fees). Open to suggestions though.

[dandanlen]

I think it would help if we break up the BoostPool struct into two items: The BoostPool (with the fee rate and amounts are available for boosts) and an individual Boost (with the pending boosts and withdrawals).

I gave it a quick try, and I don't think this is a clear improvement. Most BoostPool's methods need to access at least two, but usually three out of the three fields:

[...]

About this - it's not just about what is accessed, it's also about communicating ownership and control flow. I can access the same data in a method call, but it makes a big difference whether it's an input or and output, and what the lifetimes are.

For boost, for example, we seem to be working with a couple of higher-level primitives: We have a pool. We can 'withdraw' some portion thereof. The pool does not have access to withdrawn funds: ownership and control has been transferred. The funds can then be returned to the pool later, or might be lost. In the former case, ownership returns to the pool, in the latter case Portion::self is consumed and we do something else. So it seems at least like the ought to be some way of breaking this down.

Whether we store these separately or within the same struct is besides the point. I just think that the types and method signatures could more accurately communicate the flow of control and ownership and it would be easier to tell at a glance that, for example, certain logic errors just aren't possible. To be clear, I didn't actually attempt the refactor and am likely over-simplifying (in particular re. how this interacts with stop_boosting), but my impression while reviewing was that I had to keep a lot of context in my head when reading the method implementations.

Also I'm not saying we have to go and change this all right now, or indeed ever. It's fine as it is.

---

Re. the naming thing - again, it doesn't have to be addressed, but I do want to make my point 😄 :

As a general rule, I don't think a function implementation should make any assumptions about 'when it should be called'. That's for the caller to decide. There are exceptions to this (ie. there are times when on_event or handle_x methods are appropriate), but IMO this isn't one of them: the boost information is all contained within the boost module and functions, they don't need any external context. The reason for not returning the funds to the pool are irrelevant to the implementation.