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

Refactoring create asset and add CI tests to cover #1112

Closed
wants to merge 6 commits into from

Conversation

yrong
Copy link
Contributor

@yrong yrong commented Jan 12, 2024

Resolves: SNO-817
Requires: Snowfork/polkadot-sdk#99

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (727728a) 75.08% compared to head (474abe0) 75.09%.
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1112      +/-   ##
==========================================
+ Coverage   75.08%   75.09%   +0.01%     
==========================================
  Files          58       58              
  Lines        2464     2465       +1     
  Branches       72       72              
==========================================
+ Hits         1850     1851       +1     
  Misses        597      597              
  Partials       17       17              
Flag Coverage Δ
rust 74.02% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@yrong yrong marked this pull request as ready for review January 15, 2024 01:49
Copy link
Contributor

@claravanstaden claravanstaden left a comment

Choose a reason for hiding this comment

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

I had a quick look, I think the polkadot-sdk companion PR should still be opened right? 😄

#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone, TypeInfo)]
pub enum Call {
/// Call create_asset on foreign assets pallet.
#[codec(index = 53)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the pallet index should stay configured in the runtime config. What happens if the ForeignAssets pallet indexes are different per runtime? I think they are the same and that is the convention, but I still think this is runtime related and shouldn't live in the snowbridge crates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if the ForeignAssets pallet indexes are different per runtime? I think they are the same and that is the convention.

Yeah, that's the precondition, checked that all the same in multiple runtimes(e.g. polkadot/kusama). I do not see a reason why they need to break this rule in the future so prefer to make things simple at this moment.

But I'm open if you all think that's a problem and will make some changes accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a test to make sure they are in line?

Copy link
Contributor

@alistair-singh alistair-singh Jan 16, 2024

Choose a reason for hiding this comment

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

Ah I see a test is added. Ignore this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Clara here, I am a bit uncomfortable with baking runtime configuration inside our pallets & primitives crate.

Secondly, I am sensitive about changing code unnecessarily at this point. Our next audit is scheduled for Feb 8, and this change will increase the scope of the audit.

I want to reserve any scope increases to accommodate for the second round of code review that the Parity bridges team is going to perform soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial intention of this PR is for the comment to add the call index to some primitives which can be referenced/checked in the AH runtime tests.

So @vgeddes do we still need this PR? I think we already have the emulated test register token to cover this use case so maybe the refactoring here and the unit test added in AH may not be necessary at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Slava wanted us to move those constants here: polkadot-sdk/bridges/primitives/chain-asset-hub-rococo. I think it is fine to move the pallet index, but not to our crates. It should still be in the polkadot-sdk, and it should be configured per testnet. I think it can actually go here too: polkadot-sdk/cumulus/parachains/common/src/rococo.rs

Copy link
Contributor Author

@yrong yrong Jan 17, 2024

Choose a reason for hiding this comment

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

Thanks Clara and that makes sense! So do you think also better to move the hard-coded weight to some common runtime config?

Also noticed that Vincent mentioned to reserve any scope increases to accommodate for the second round of code review then not sure if it's the right time to do the refactoring.

So I'll convert the PR as draft and let the team decide whether to continue with the change or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@claravanstaden Some refactoring in 37aa97e as you suggested.

@yrong yrong marked this pull request as draft January 17, 2024 04:21
@yrong yrong marked this pull request as ready for review January 17, 2024 12:25
@yrong yrong changed the title Runtime test for create asset in AH Refactoring the call of create asset and add CI tests to cover Jan 18, 2024
@yrong yrong changed the title Refactoring the call of create asset and add CI tests to cover Refactoring create asset and add CI tests to cover Jan 18, 2024
Comment on lines +86 to +97
/// CallInfo required to create the foreign asset
#[derive(Clone, Encode, Decode, RuntimeDebug)]
pub struct CreateAssetCallInfo {
/// Index of the create call
pub call_index: CallIndex,
/// Deposit required to create the asset
pub asset_deposit: u128,
/// Minimal balance of the asset
pub min_balance: u128,
/// Weight required for the xcm transact
pub transact_weight_at_most: Weight,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we add a CallInfo structure for the create asset and make it runtime config(without any hard coded params anymore), meanwhile we add runtime tests in asset-hub to check the call_index and transact_weight_at_most all as expected.

Copy link
Collaborator

@vgeddes vgeddes left a comment

Choose a reason for hiding this comment

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

Hey Ron this is a very nice improvement. However we have an audit in 2-3 weeks and I want us to have some sort of code freeze for relatively non-important changes.

So lets wait till after the next audit to merge this in.

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

Successfully merging this pull request may close these issues.

4 participants