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

pallet-asset: Added new test-case for DistributionLimitExceeded. #522

Merged
merged 10 commits into from
Jan 6, 2025

Conversation

ritankarsaha
Copy link
Contributor

This PR fixes issue #379 .

This issue has been structured similarly to the asset_create_should_succeed test.
This test will ensure that an asset can be issued successfully and will include coverage for reaching and exceeding the MaxAssetDistribution limit.

  • Test Initialization:
    The test sets up a space, approves it, and creates an asset similarly to the asset_create_should_succeed test.
  • Asset Issuance:
    The loop issues the asset up to the MaxAssetDistribution limit (set to 25 for testing). Each iteration simulates issuing the asset to a different recipient.
  • Error Handling:
    After the limit is reached, the test tries to issue the asset to one more recipient, which should trigger the DistributionLimitExceeded error. This ensures that the pallet's logic correctly handles and enforces the distribution limit.

This implementation checks that the asset issuance process is valid up to the MaxAssetDistribution limit and correctly throws an error when the limit is exceeded.

Copy link
Member

@vatsa287 vatsa287 left a comment

Choose a reason for hiding this comment

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

This might fail with asset_id is not found.

pallets/asset/src/tests.rs Outdated Show resolved Hide resolved
pallets/asset/src/tests.rs Outdated Show resolved Hide resolved
pallets/asset/src/tests.rs Outdated Show resolved Hide resolved
@vatsa287
Copy link
Member

@ritankarsaha You have to update the constant which defines the max allowed value for the test to evaluate against. You can find the mock value for the constant set mock.rs of pallet-asset.

@ritankarsaha
Copy link
Contributor Author

@ritankarsaha You have to update the constant which defines the max allowed value for the test to evaluate against. You can find the mock value for the constant set mock.rs of pallet-asset.

@vatsa287
To implement this in mock.rs I am changing -

    parameter_types! {
	pub const MaxEncodedValueLength: u32 = 1_024;
	pub const MaxAssetDistribution: u32 = u32::MAX;
}

to

parameter_types! {
	pub const MaxEncodedValueLength: u32 = 1_024;
	pub const MaxAssetDistribution: u32 = 25;
}

Is this implementation correct ?

@ritankarsaha
Copy link
Contributor Author

@vatsa287 Could you give me an insight on why is it failing on rustfmt ci test all times. I have followed all rust formatting guidelines and am also using the necessary extensions required for this. I had also formatted the specific file tests.rs with rustfmt tests.rs

@vatsa287
Copy link
Member

-use crate::mock::*;
-use crate::{types::AssetIssuanceEntry, Error};
+use crate::{mock::*, types::AssetIssuanceEntry, Error};

Above changes is needed as per fmt.

Run cargo +nightly fmt --all and re-commit.

@ritankarsaha
Copy link
Contributor Author

-use crate::mock::*;
-use crate::{types::AssetIssuanceEntry, Error};
+use crate::{mock::*, types::AssetIssuanceEntry, Error};

Above changes is needed as per fmt.

Run cargo +nightly fmt --all and re-commit.

@vatsa287 I have made the following changes:-

From

use super::*;
use crate::mock::*;
use crate::{types::AssetIssuanceEntry, Error};

To

use super::*;
use crate::{mock::*, types::AssetIssuanceEntry, Error};

I hope this is correct, however on running the command cargo +nightly fmt --all there were no changes in the formatting of the code and the current format stayed. So i guess the error was only in the above lines and not in formatting.
I am pushing this new commit into the PR.

@ritankarsaha
Copy link
Contributor Author

@vatsa287 For the clippy test fail,

error: unused import: `assert_noop`
 --> pallets/asset/src/tests.rs:5:33
  |
5 | use frame_support::{assert_err, assert_noop, assert_ok, BoundedVec};
  |                                 ^^^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
  = help: to override `-D warnings` add `#[allow(unused_imports)]`

error: could not compile `pallet-asset` (lib test) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

I am removing the unused import assert_noop, I guess this is the error which is failing the clippy test

For the test failing should i need to consider anything else?

@vatsa287
Copy link
Member

Instead of waiting for CI always since it takes a bit of time, you can run the required tests at your local setup.
Check below.
https://github.com/dhiway/cord/?tab=readme-ov-file#before-submitting-the-pr

@ritankarsaha
Copy link
Contributor Author

Screenshot 2024-12-27 at 2 53 25 PM

@vatsa287 fixed the changes and pushed a new commit into the PR. Tests also have successfully ran locally.

@ritankarsaha
Copy link
Contributor Author

ritankarsaha commented Dec 31, 2024

@vatsa287 ran the cargo +nightly fmt --all test to remove the rustfmt error and pushed a new commit without the formatting error.

@ritankarsaha
Copy link
Contributor Author

ritankarsaha commented Jan 3, 2025

@vatsa287 is this implementation correct? The CI tests for this have also passed locally.

let exceeding_digest =
<Test as frame_system::Config>::Hashing::hash(&exceeding_entry.encode()[..]);

assert_noop!(
Copy link
Member

Choose a reason for hiding this comment

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

LGTM.

Change to assert_err!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved this error @vatsa287
and the test case passed.

Screenshot 2025-01-03 at 2 46 33 PM

Copy link
Member

@vatsa287 vatsa287 left a comment

Choose a reason for hiding this comment

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

LGTM

@amarts amarts merged commit accb726 into dhiway:develop Jan 6, 2025
3 checks passed
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.

3 participants