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 pallet Assets Chain Extension #1124

Merged
merged 10 commits into from
Jan 10, 2024
Merged

Add pallet Assets Chain Extension #1124

merged 10 commits into from
Jan 10, 2024

Conversation

PierreOssun
Copy link
Member

@PierreOssun PierreOssun commented Jan 3, 2024

Pull Request Summary
Added Pallet Assets Chain extension to Shibuya (and local).

It expose the same API as assets-er20 precompiles:
State modify functions

  • Transfer
  • TransferApproved
  • Mint
  • Burn
  • ApproveTransfer

Getters

  • BalanceOf
  • TotalSupply
  • MinimumBalance
  • MetadataName
  • MetadataSymbol
  • MetadataDecimal

Error Handling
Chain extension return a RetVal that is being handled/decoded in contract to get proper Error handling.

Weight
The chain extension reads scale encoded bytes from buffer to get the type. Every call only use fixed bytes (no usage of read_as_unbunded() to read Vectors) so it can not be abused. It then charge the pallet call and perform the call. It then return an RetVal in case of a extrinsic call (no encoding) or write the encoded bytes in buffer in case of query.

Tests

  • Mock Unit tests
  • Integration tests

Test contract: AstarNetwork/ink-test-contracts#4

@PierreOssun PierreOssun added the runtime This PR/Issue is related to the topic “runtime”. label Jan 3, 2024
@PierreOssun PierreOssun added the shibuya related to shibuya label Jan 3, 2024
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

Looks good!

Few minor comments, the most impactful probably the one about weight charging placement.

chain-extensions/pallet-assets/src/mock.rs Outdated Show resolved Hide resolved
chain-extensions/pallet-assets/src/lib.rs Outdated Show resolved Hide resolved
chain-extensions/pallet-assets/src/lib.rs Outdated Show resolved Hide resolved
tests/integration/src/assets_chain_extensions.rs Outdated Show resolved Hide resolved
Comment on lines +54 to +60
assert_ok!(Assets::set_team(
RuntimeOrigin::signed(ALICE),
ASSET_ID,
addr.clone(),
ALICE,
ALICE
));
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity - is this needed for the test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is needed. Only EOA can create assets (because create is not part of CE) and set_team is used to transfer Admin roles. As in Chain extension the Origin can only be the contract, Issuer (minter role) should be transferred to it in order to mint assets.
I move the comment line on top of it for more clarity

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, I missed the addr.clone(), my bad!

Copy link
Member

@Dinonard Dinonard Jan 4, 2024

Choose a reason for hiding this comment

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

One thing you could do in test is use force_create, that way you can set the owner to whoever in a single call.

This is informational, no changes needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes true! I tend to be closer to real use case in my tests

amount: u128,
) -> Result<ExecReturnValue, DispatchError> {
let data = [
[0x84, 0xa1, 0x5d, 0xa1].to_vec(),
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to reuse the consts here?

Copy link
Member Author

Choose a reason for hiding this comment

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

functions selector are the first 4bytes of the blake_2b hash of the function name. So it cannot be evaluated at compile time. For more clarity I added a macro that takes the &str of the function name to be more straight forward and not deal with bytes arrays

Copy link
Member

Choose a reason for hiding this comment

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

I see, but I didn't mean for them to be evaluated at compile time necessarily, the question was more generic about reusing the constants. They could also be str constants, and macros could be used on them, I guess.

But I think you got my point 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah got your point but I responded there to you comment above Question - can these values be fetched automatically or calculated from a string/tuple? so I made things not so clear sorry
To respond to the str const:
The str const will be 4 bytes len str of an hash so not more informative/less error prone than explicit bytes array. I think with macro is the best approach here.

Dinonard
Dinonard previously approved these changes Jan 4, 2024
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +54 to +60
assert_ok!(Assets::set_team(
RuntimeOrigin::signed(ALICE),
ASSET_ID,
addr.clone(),
ALICE,
ALICE
));
Copy link
Member

@Dinonard Dinonard Jan 4, 2024

Choose a reason for hiding this comment

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

One thing you could do in test is use force_create, that way you can set the owner to whoever in a single call.

This is informational, no changes needed.

amount: u128,
) -> Result<ExecReturnValue, DispatchError> {
let data = [
selector_bytes!("transfer").to_vec(),
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick - these strings could have been left as &str consts.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean like const TRASNFER &str = b"9bae9d5e"; ?
I think this way is more readable (even tho it's not the most performant - but it's off chain tests)

Copy link
Member

Choose a reason for hiding this comment

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

Almost, I meant like `const TRANSFER: &str = "transfer";

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah but I would have to hash that &str and get the first 4 bytes so it defeats the static part

Copy link
Member

@ashutoshvarma ashutoshvarma left a comment

Choose a reason for hiding this comment

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

Looks good! Just some nitpick comments

chain-extensions/pallet-assets/src/lib.rs Outdated Show resolved Hide resolved
chain-extensions/pallet-assets/src/tests.rs Show resolved Hide resolved
Copy link

Code Coverage

Package Line Rate Branch Rate Health
pallets/xc-asset-config/src 53% 0%
chain-extensions/types/xvm/src 0% 0%
precompiles/dapp-staking-v3/src/test 0% 0%
precompiles/dapps-staking/src 94% 0%
precompiles/substrate-ecdsa/src 74% 0%
precompiles/assets-erc20/src 81% 0%
pallets/dynamic-evm-base-fee/src 81% 0%
precompiles/unified-accounts/src 100% 0%
primitives/src/xcm 66% 0%
pallets/static-price-provider/src 61% 0%
pallets/unified-accounts/src 84% 0%
pallets/dapp-staking-v3/src 76% 0%
pallets/dapps-staking/src 81% 0%
pallets/dapps-staking/src/pallet 85% 0%
precompiles/xvm/src 74% 0%
pallets/dapp-staking-v3/rpc/runtime-api/src 0% 0%
chain-extensions/unified-accounts/src 0% 0%
pallets/inflation/src 70% 0%
primitives/src 66% 0%
chain-extensions/xvm/src 0% 0%
chain-extensions/types/assets/src 0% 0%
precompiles/xcm/src 72% 0%
pallets/dapp-staking-v3/src/benchmarking 0% 0%
pallets/dapp-staking-v3/src/test 0% 0%
pallets/collator-selection/src 69% 0%
chain-extensions/pallet-assets/src 56% 0%
precompiles/dapp-staking-v3/src 90% 0%
pallets/block-rewards-hybrid/src 87% 0%
chain-extensions/types/unified-accounts/src 0% 0%
pallets/dapp-staking-migration/src 39% 0%
pallets/xvm/src 40% 0%
precompiles/sr25519/src 64% 0%
pallets/ethereum-checked/src 48% 0%
Summary 70% (3254 / 4631) 0% (0 / 0)

Minimum allowed line rate is 50%

@PierreOssun PierreOssun merged commit 4dbe881 into master Jan 10, 2024
8 checks passed
@PierreOssun PierreOssun deleted the feat/pallet-assets-ce branch January 10, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”. shibuya related to shibuya
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants