feature/plmc-306-fix-testsbenchmarks-for-new-bucket-system #93
Conversation
…emove some duplicate code
PLMC-306 Fix tests/benchmarks for new Bucket System
|
There was a problem hiding this comment.
At present, the benchmarks cannot be compiled due to a missing parameter in the calculate_auction_plmc_spent within the bid benchmark. I've provided a suggestion in the comments to address this.
Aside from that, I see this as a significant improvement and a step towards more rigorous testing.
Regarding the changes inside the instantiator I would like to get a second feedback from @JuaniRios
@vstam1: Can you also provide a brief overview of the changes in the PR description?
pallets/funding/src/instantiator.rs
Outdated
| } | ||
| output | ||
| } | ||
|
|
||
| pub fn calculate_auction_funding_asset_spent(bids: Vec<BidParams<T>>) -> Vec<UserToStatemintAsset<T>> { | ||
| pub fn simulate_bids_with_bucket(bids: Vec<BidParams<T>>, metadata: &ProjectMetadataOf<T>) -> Vec<BidParams<T>> { |
There was a problem hiding this comment.
So if I understand correctly, our instantiator can only calculate auction PLMC and USDT when the whole bid amount fits in one bucket, so we need to split down these bids to neatly fit in buckets with this function?
If that's the case, we should also test that the behaviour of bucket splitting works on-chain
There was a problem hiding this comment.
Our current bucket logic in pallet-funding does exactly this split down. So I tried to keep most functions as is in the instantiator, and keep the idea of BidParams having a price. The actual price for the BidParams is calculated by the simulate_bids_with_bucket. We could still pass the original bids to the actual bidding functionality or to the bid_for_users function, as it does all the price calculation there. So it doesn't really matter if we pre split the bids or let the funding pallet logic split the bids. However, for calculating the funding/plmc assets necessary for the operation, we do have to pre calculate the actually used amount, and for that we do have to precalculate the price. I was thinking about removing the price parameter and add a ticket_size parameter, however for eventually calculating which bids made it into the auction, we have to know which parts of a bid where pushed into a new bucket. So the split in simulate_bids_with_bucket was useful to simplify the filter_bids_after_auction function
There was a problem hiding this comment.
Let's use the on-chain bucket instead of assuming always a zero bucket
| @@ -1403,6 +1484,11 @@ pub trait Accounts { | |||
|
|
|||
| fn accounts(&self) -> Vec<Self::Account>; | |||
| } | |||
|
|
|||
| pub trait AccountMerge { | |||
There was a problem hiding this comment.
I like this approach, but it's duplicating the same logic as merge_add_mappings_by_user.
There was a problem hiding this comment.
How I understood merge_add_mappings_by_user it does not merge the duplicate accounts in the first vector. So if the first vector contains [alice, alice, bob] then it returns [alice, alice, bob]. But I might be mistaken.
Edit: yes it keeps the duplicates. We could choose to merge the duplicates as well. What do you think @JuaniRios
There was a problem hiding this comment.
Oh, I do not think that keeping duplicates on the first vector is intended. Let's test if fixing that breaks anything, and add that feature.
The only difference between .merge_accounts() and merge_add_mappings_by_user seems to be that one works for a single vector, and the other one for a vector of vectors.
I do like your solution more though, and if we find a way to also do vector of vectors we could completely replace the old one.
Do you think we should keep the old merge for vectors, or do something like a generic impl of merge_accounts for vector of vectors?
Open to suggestions
pallets/funding/src/instantiator.rs
Outdated
| LockType::Participation(project_id), | ||
| ); | ||
| self.do_bid_transferred_statemint_asset_assertions(funding_asset_deposits.merge_accounts(), project_id); | ||
| self.do_free_plmc_assertions(expected_free_plmc_balances.merge_accounts()); |
There was a problem hiding this comment.
Already merged with merge_add_mappings_by_user
pallets/funding/src/instantiator.rs
Outdated
| self.do_free_plmc_assertions(expected_free_plmc_balances); | ||
| self.do_free_statemint_asset_assertions(prev_funding_asset_balances); | ||
| self.do_reserved_plmc_assertions( | ||
| total_plmc_participation_locked.merge_accounts(), |
There was a problem hiding this comment.
already merged with merge_add_mappings_by_user
pallets/funding/src/instantiator.rs
Outdated
| funding_asset_deposits.merge_accounts(), | ||
| project_id, | ||
| ); | ||
| self.do_free_plmc_assertions(expected_free_plmc_balances.merge_accounts()); |
pallets/funding/src/instantiator.rs
Outdated
| self.do_free_plmc_assertions(expected_free_plmc_balances); | ||
| // self.do_free_statemint_asset_assertions(prev_funding_asset_balances); | ||
| self.do_reserved_plmc_assertions( | ||
| total_plmc_participation_locked.merge_accounts(), |
pallets/funding/src/instantiator.rs
Outdated
| funding_asset_deposits.merge_accounts(), | ||
| project_id, | ||
| ); | ||
| self.do_free_plmc_assertions(expected_free_plmc_balances.merge_accounts()); |
pallets/funding/src/tests.rs
Outdated
| glutton_bid_1.clone(), | ||
| glutton_bid_2.clone(), | ||
| ]); | ||
| let bids = MockInstantiator::simulate_bids_with_bucket( |
There was a problem hiding this comment.
We should let the protocol split the bids, and not do it ourselves before submitting the extrinsic. Although I understand that calculate_auction_plmc_spent and funding_spent do not support the calculation across multiple buckets.
Making it support it now and avoiding pre-computing the extrinsic inputs will save us future headaches
|
/bot test pallet-funding |
|
test: Succeeded! ✅ |
|
/bot benchmark pallet-funding |
|
benchmark: Succeeded! ✅ |
|
/bot test pallet-funding |
|
/bot benchmark pallet-funding |
|
benchmark: Succeeded! ✅ |
|
test: Succeeded! ✅ |
lrazovic
deleted the
feature/plmc-306-fix-testsbenchmarks-for-new-bucket-system
branch
No description provided.