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

[C4GT] Asset: Add tests for DistributionLimitExceeded #379

Open
3 tasks
vatsa287 opened this issue Mar 12, 2024 · 13 comments
Open
3 tasks

[C4GT] Asset: Add tests for DistributionLimitExceeded #379

vatsa287 opened this issue Mar 12, 2024 · 13 comments
Assignees
Labels

Comments

@vatsa287
Copy link
Member

vatsa287 commented Mar 12, 2024

Description

Subtask under : cord-network/community#7

New testcase can be tested under cargo test -p pallet-asset after adding it in the code.

Goals

  • Add tests for DistributionLimitExceeded for pallet/asset

Expected Outcome

  • Test should assert for DistributionLimitExceeded being returned properly in all the possible calls.
  • If there are more than one function returning this error code, all of them should be validated either as one test case or as multiple test cases.

Acceptance Criteria

NA

Implementation Details

Look at other test cases, and add a test case for the same.
Ex: check method asset_create_should_succeed which validates creation of asset.

Mockups / Wireframes

NA


Product Name

CORD

Organization Name

Dhiway

Domain

Blockchain

Tech Skills Needed

Rust

Mentor(s)

@amarts @vatsa287

Complexity

[Medium]

Category

[Test]

Sub Category

[Beginner friendly]

@vnitin08
Copy link

Hey @vatsa287 , I would like to work on this issue.

@zeel991
Copy link
Contributor

zeel991 commented Mar 29, 2024

Hey @vatsa287 , I would like to work on this issue.

Hey Vinit , you can raise the PR , if everything seems right , you will be assigned .

@kartikeshwar156
Copy link

hey @vatsa287 I would like to work on this issue

@PriyaD17
Copy link

Hey @vatsa287 @amarts I have read the code and understood the idea to approach the issue, and I would like to work on this issue

@VedantKhairnar
Copy link

Hello @PriyaD17
Any update on the contribution?
Pls do let us know if you need any assistance.
Thanks.

@PriyaD17
Copy link

Hey @VedantKhairnar Thanks for checking in, just got busy in exams and and all, will def reach out if I get stuck anywhere, once again thanks for asking

@hp77-creator
Copy link
Contributor

Hi @vatsa287
Would like to work on this, but needed to understand, what does Distribution mean? what are assets? Is there any design doc for this?

@vatsa287
Copy link
Member Author

^^ @hp77-creator

Distribution Limit here refers to the amount of asset token which can be issued to various entities.

To fix this,

  1. Update the MaxAssetDistribution amount to a value which can be breached. Say 25.
parameter_types! {
	pub const MaxEncodedValueLength: u32 = 1_024;
	pub const MaxAssetDistribution: u32 = u32::MAX;
}
  1. Try calling the issue and vc_issue because these 2 are the functions which emit these errors.
    When a asset is distributed for more than 25 entities this error should be thrown.

@ritankarsaha
Copy link

@vatsa287 I believe this needs to be done in 2 steps. Firstly, setting up the new parameter for testing. In the code, updating the MaxAssetDistribution to a testable value, say 25. This will allow us to easily hit the distribution limit when writing the test.
Secondly, Implementing the Test Case for DistributionLimitExceeded. This includes Creating a test in pallet-asset under the test module. This test should call the issue and vc_issue functions until the distribution limit is reached, triggering the DistributionLimitExceeded error.

Ig this enough? Should I start working on the issue or is there anything else I need to consider as well @vatsa287 ?

@vatsa287
Copy link
Member Author

vatsa287 commented Nov 7, 2024

@ritankarsaha
Yes correct.
You can start working on this. Please refer the comment above, consider the functions which emit these errors.

@ritankarsaha
Copy link

@ritankarsaha Yes correct. You can start working on this. Please refer the comment above, consider the functions which emit these errors.

Sure @vatsa287

@ritankarsaha
Copy link

I have fixed this issue and opened a PR at #552

@ritankarsaha
Copy link

@vatsa287 I had a doubt while I was going through the tests.rs codebase.

The constants DID_00, DID_01, and ACCOUNT_00 are fine, but it’s worth noting that DID_00 and DID_01 currently use the same AccountId32.
If DID_00 and DID_01 are meant to be identical, that's okay; otherwise, you might want to differentiate them as shown below-

pub(crate) const DID_00: SubjectId = SubjectId(AccountId32::new([1u8; 32]));
pub(crate) const DID_01: SubjectId = SubjectId(AccountId32::new([2u8; 32])); // just for demo purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants