Skip to content
This repository has been archived by the owner on Oct 2, 2023. It is now read-only.

Refactor use generic AccountId type #137

Merged
merged 150 commits into from
Aug 31, 2023
Merged

Conversation

magecnion
Copy link
Contributor

@magecnion magecnion commented Aug 29, 2023

The main purpose of this task is to replace the specific type H160, previously used in the pallet to store asset ownership addresses. To achieve this, two traits have been introduced into the runtime:

  • AccountMapping: Manages the conversion between AccountId and H160, primarily for use within the precompiles module.
  • AssetIdToAddress: Returns the initial owner corresponding to the asset_id.

@magecnion magecnion marked this pull request as ready for review August 29, 2023 08:12
@magecnion magecnion requested review from asiniscalchi and a user August 29, 2023 08:12
@magecnion magecnion linked an issue Aug 29, 2023 that may be closed by this pull request
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

It looks great in general

assert_ne!(asset1, asset2);

// check asset in decimal format
assert_eq!(
Copy link

Choose a reason for hiding this comment

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

I don't think this is necessary. it tests the functionality of U256 which is outside of our crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would leave because it is useful when you test through remix as you need number in decimal format

Copy link

Choose a reason for hiding this comment

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

this is not addressed from last review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not addressed from last review

I suggest to leave this assert because it's useful when testing remix

pallets/living-assets-ownership/src/lib.rs Outdated Show resolved Hide resolved
pallets/living-assets-ownership/src/lib.rs Outdated Show resolved Hide resolved
pallets/living-assets-ownership/src/traits.rs Outdated Show resolved Hide resolved
pallets/living-assets-ownership/src/traits.rs Outdated Show resolved Hide resolved
pallets/living-assets-ownership/src/traits.rs Outdated Show resolved Hide resolved
runtime/src/lib.rs Outdated Show resolved Hide resolved
runtime/src/lib.rs Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

lgtm now

pallets/living-assets-ownership/Cargo.toml Show resolved Hide resolved
pallets/living-assets-ownership/src/lib.rs Outdated Show resolved Hide resolved
@magecnion magecnion merged commit 38d40fa into dev Aug 31, 2023
@magecnion magecnion deleted the refactor/use_generic_accountid branch August 31, 2023 08:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERC721.transferFrom
2 participants