Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MutatesHold trait to pallet asset #1

Closed
wants to merge 1 commit into from

Conversation

darkforest0202
Copy link
Collaborator

@darkforest0202 darkforest0202 commented Oct 19, 2023

TODO: if we want to PR to be accepted we must add significantly more testing

@darkforest0202 darkforest0202 changed the title Add MutatesHold trait to pallet asset.#315 Add MutatesHold trait to pallet asset Oct 19, 2023
@olanod olanod force-pushed the feat-add-hold-traits-pallet-asset branch from 40766b4 to 640ce7c Compare November 21, 2023 10:23
pandres95 pushed a commit that referenced this pull request Feb 15, 2024
1. Benchmark results are collected in a single struct.
2. The output of the results is prettified.
3. The result struct used to save the output as a yaml and store it in
artifacts in a CI job.

```
$ cargo run -p polkadot-subsystem-bench --release -- test-sequence --path polkadot/node/subsystem-bench/examples/availability_read.yaml | tee output.txt
$ cat output.txt

polkadot/node/subsystem-bench/examples/availability_read.yaml #1

Network usage, KiB                     total   per block
Received from peers               510796.000  170265.333
Sent to peers                        221.000      73.667

CPU usage, s                           total   per block
availability-recovery                 38.671      12.890
Test environment                       0.255       0.085


polkadot/node/subsystem-bench/examples/availability_read.yaml #2

Network usage, KiB                     total   per block
Received from peers               413633.000  137877.667
Sent to peers                        353.000     117.667

CPU usage, s                           total   per block
availability-recovery                 52.630      17.543
Test environment                       0.271       0.090


polkadot/node/subsystem-bench/examples/availability_read.yaml #3

Network usage, KiB                     total   per block
Received from peers               424379.000  141459.667
Sent to peers                        703.000     234.333

CPU usage, s                           total   per block
availability-recovery                 51.128      17.043
Test environment                       0.502       0.167

```

```
$ cargo run -p polkadot-subsystem-bench --release -- --ci test-sequence --path polkadot/node/subsystem-bench/examples/availability_read.yaml | tee output.txt
$ cat output.txt
- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml #1'
  network:
  - resource: Received from peers
    total: 509011.0
    per_block: 169670.33333333334
  - resource: Sent to peers
    total: 220.0
    per_block: 73.33333333333333
  cpu:
  - resource: availability-recovery
    total: 31.845848445
    per_block: 10.615282815
  - resource: Test environment
    total: 0.23582828799999941
    per_block: 0.07860942933333313

- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml #2'
  network:
  - resource: Received from peers
    total: 411738.0
    per_block: 137246.0
  - resource: Sent to peers
    total: 351.0
    per_block: 117.0
  cpu:
  - resource: availability-recovery
    total: 18.93596025099999
    per_block: 6.31198675033333
  - resource: Test environment
    total: 0.2541994199999979
    per_block: 0.0847331399999993

- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml #3'
  network:
  - resource: Received from peers
    total: 424548.0
    per_block: 141516.0
  - resource: Sent to peers
    total: 703.0
    per_block: 234.33333333333334
  cpu:
  - resource: availability-recovery
    total: 16.54178526900001
    per_block: 5.513928423000003
  - resource: Test environment
    total: 0.43960946299999537
    per_block: 0.14653648766666513
```

---------

Co-authored-by: Andrei Sandu <[email protected]>
@@ -86,7 +86,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
// allow accidental usage of all consumer references which could cause grief.
if !frame_system::Pallet::<T>::can_inc_consumer(who) {
frame_system::Pallet::<T>::dec_consumers(who);
return Err(Error::<T, I>::UnavailableConsumer.into())
return Err(Error::<T, I>::UnavailableConsumer.into());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This is a formatting error. Ignore for future re-implementation.

@@ -133,24 +133,24 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
None => return DepositConsequence::UnknownAsset,
};
if increase_supply && details.supply.checked_add(&amount).is_none() {
return DepositConsequence::Overflow
return DepositConsequence::Overflow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this.

}
if let Some(account) = Account::<T, I>::get(id, who) {
if account.status.is_blocked() {
return DepositConsequence::Blocked
return DepositConsequence::Blocked;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

}
if account.balance.checked_add(&amount).is_none() {
return DepositConsequence::Overflow
return DepositConsequence::Overflow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

}
} else {
if amount < details.min_balance {
return DepositConsequence::BelowMinimum
return DepositConsequence::BelowMinimum;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

}
if !details.is_sufficient && !frame_system::Pallet::<T>::can_accrue_consumers(who, 2) {
return DepositConsequence::CannotCreate
return DepositConsequence::CannotCreate;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

}
if details.is_sufficient && details.sufficients.checked_add(1).is_none() {
return DepositConsequence::Overflow
return DepositConsequence::Overflow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

@@ -170,20 +170,20 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
None => return UnknownAsset,
};
if details.supply.checked_sub(&amount).is_none() {
return Underflow
return Underflow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

}
if details.status == AssetStatus::Frozen {
return Frozen
return Frozen;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

}
if amount.is_zero() {
return Success
return Success;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

}
let account = match Account::<T, I>::get(&id, who) {
Some(a) => a,
None => return BalanceLow,
};
if account.status.is_frozen() {
return Frozen
return Frozen;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

@@ -267,7 +267,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Ok(dust) => actual.saturating_add(dust), //< guaranteed by reducible_balance
Err(e) => {
debug_assert!(false, "passed from reducible_balance; qed");
return Err(e)
return Err(e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

@@ -360,7 +360,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
debug_assert!(false, "refund did not result in dead account?!");
// deposit may have been refunded, need to update `Account`
Account::<T, I>::insert(id, &who, account);
return Ok(())
return Ok(());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

@@ -391,12 +391,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
debug_assert!(false, "refund did not result in dead account?!");
// deposit may have been refunded, need to update `Account`
Account::<T, I>::insert(&id, &who, account);
return Ok(())
return Ok(());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

}
Asset::<T, I>::insert(&id, details);
// Executing a hook here is safe, since it is not in a `mutate`.
T::Freezer::died(id, &who);
return Ok(())
return Ok(());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this.

@pandres95
Copy link
Member

Closed in favour of paritytech#4530

@pandres95 pandres95 closed this Sep 17, 2024
@pandres95 pandres95 deleted the feat-add-hold-traits-pallet-asset branch September 17, 2024 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants