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

precompile erc 721 only owner_of #104

Merged
merged 61 commits into from
Aug 8, 2023
Merged

Conversation

asiniscalchi
Copy link
Member

No description provided.

@asiniscalchi asiniscalchi changed the title Feature/precompile erc 721 precompile erc 721 Aug 3, 2023
@asiniscalchi asiniscalchi linked an issue Aug 3, 2023 that may be closed by this pull request
@asiniscalchi asiniscalchi changed the title precompile erc 721 precompile erc 721 only tockenURI Aug 3, 2023
Copy link
Contributor

@tonimateos tonimateos left a comment

Choose a reason for hiding this comment

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

Minor comments, plus the View function requirement

precompile/erc721/contracts/IERC721.sol Outdated Show resolved Hide resolved
precompile/erc721/src/lib.rs Outdated Show resolved Hide resolved
precompile/erc721/src/tests.rs Show resolved Hide resolved
pallets/living-assets-ownership/Cargo.toml Show resolved Hide resolved
}
}

pub fn collection_id_to_address(collection_id: CollectionId) -> H160 {
Copy link

Choose a reason for hiding this comment

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

this is really unidiomatic for Rust and Substrate, generally. Either create a trait for all these functions, sth like LivingAssetsUtils and implement this trait for Pallet or to some dummy struct and use them to call these functions or just move these functions under impl<T: Config> Pallet<T> {} as static functions.

they are also missing comments

Copy link
Member Author

Choose a reason for hiding this comment

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

it'll be adressed : #116

pallets/living-assets-ownership/src/lib.rs Outdated Show resolved Hide resolved
pallets/living-assets-ownership/src/lib.rs Outdated Show resolved Hide resolved
@@ -25,3 +27,7 @@ pub trait CollectionManager<AccountId> {
/// Create collection
fn create_collection(owner: AccountId) -> Result<CollectionId, &'static str>;
}

Copy link

Choose a reason for hiding this comment

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

documentation

Copy link
Member Author

Choose a reason for hiding this comment

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

sgtm

Copy link

Choose a reason for hiding this comment

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

it is missing documentation. you can use the comments to leave TODO mark for complete implementation of ERC721 as well.

pallets/living-assets-ownership/src/tests.rs Outdated Show resolved Hide resolved
runtime/Cargo.toml Show resolved Hide resolved
@asiniscalchi asiniscalchi linked an issue Aug 8, 2023 that may be closed by this pull request
@@ -25,3 +27,7 @@ pub trait CollectionManager<AccountId> {
/// Create collection
fn create_collection(owner: AccountId) -> Result<CollectionId, &'static str>;
}

Copy link

Choose a reason for hiding this comment

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

it is missing documentation. you can use the comments to leave TODO mark for complete implementation of ERC721 as well.

@@ -25,3 +27,7 @@ pub trait CollectionManager<AccountId> {
/// Create collection
fn create_collection(owner: AccountId) -> Result<CollectionId, &'static str>;
}

pub trait Erc721 {
fn owner_of(collection_id: CollectionId, asset_id: U256) -> Result<H160, &'static str>;
Copy link

Choose a reason for hiding this comment

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

also forgot to mention that it's not a good practice to use str for errors, try to use explicit Error type which you can pass to the trait or type.

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 totally agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

it will be addressed here : #116

@@ -25,3 +27,7 @@ pub trait CollectionManager<AccountId> {
/// Create collection
fn create_collection(owner: AccountId) -> Result<CollectionId, &'static str>;
}

pub trait Erc721 {
fn owner_of(collection_id: CollectionId, asset_id: U256) -> Result<H160, &'static str>;
Copy link

Choose a reason for hiding this comment

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

Suggested change
fn owner_of(collection_id: CollectionId, asset_id: U256) -> Result<H160, &'static str>;
type Error;
fn owner_of(collection_id: CollectionId, asset_id: U256) -> Result<H160, &'static str>;

Copy link

Choose a reason for hiding this comment

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

sth like this

Copy link
Member Author

Choose a reason for hiding this comment

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

precompile/living-assets/src/tests.rs Show resolved Hide resolved
runtime/Cargo.toml Show resolved Hide resolved
@asiniscalchi asiniscalchi merged commit 6a0b2ce into dev Aug 8, 2023
@asiniscalchi asiniscalchi deleted the feature/precompile_erc_721 branch August 8, 2023 13:20
asiniscalchi added a commit that referenced this pull request Aug 9, 2023
* Delete polkadot-launch directory

* Remove `template` mentions from runtime

* Fix clippy

* first hello world test

* fmt

* checking the selectors

* testdata updated

* renaming

* added testdata for owner_of

* removed a type declaration

* fmt

* using macro to help using behavior pattern testing

* usign macro

* refactoring

* refactoring

* main tests covered

* name refactoring

* removed unused dependency

* removed testdata

* add test on calling unexsiting selector

* removed unused code

* fix returning errors message

* fmt

* adding explicit value transferred in tests, and making explicit both methods supported

* removing unreachable path

* added tests that the implemented methods are non-payable

* using closures

* refactoring

* declare name in temoplate

* refactoring PrecompileMock -> Mock

* created handle_from_input

* added docuemntation

* renaming: define_precompile_mock -> define_precompile_mock_simple

* renaming: define_precompile_mock_closure -> impl_precompile_mock

* name refactoring

* refactoring: handle_from_input use create_mock_handle

* refactoring: handle_from_input -> create_mock_handle_from_input

* comment the inputs

* using constants

* first try

* add create collection returning id to trati

* fix compilation

* trait crate_collection2 returns Result

* test on create collection failure

* create collection returns an id

* precompiled return from pallet

* added create_collection returning id in solidity interface

* assign caller as owner

* remove legacy create collection in precompile

* removed create_collection2 from trait

* compiling

* compiling

* create collection extrinsic create consecutive collections

* removed legacy code from solidity interface

* create file traits.rs

* trait LivingAssetOwnership -> CollectionManager

* doc: solidity

* documentation

* better tests

* better testing

* fix typo

* conversion collection_id to address

* removed owner of from precompile

* adding test that create_collection is non-payable

* checking err is as expected: "non payable"

* fixing a test that was failing for a different reason, as shown by testing that the error received was as expected

* fix compilation

* fix compilation

* adding event

* added event Create Collection

* fmt

* copied MockHandle to implement log function

* GRANDPA bridge (#106)

* Only client side compile errors

* Upgrade to v1.0.0

* Add grandpa bridge to runtime

* Fix clippy and tests

* Fix lints

* Fix tests and clippy

* Remove comment

* test log selector

* remove dependency

* Mock logs logs

* testing event

* removed duplicated test

* using succeed utility

* collection_it_to_addressis private

* return value is correctly encoded as a n Address (#109)

* return value is correctly encoded as a n Address

* removed outdated comment

* fix the documentation

* precompile erc 721 only owner_of (#104)

* create mock and testing

* first test

* testing ethereum reserved addresses

* refactoring

* refactoring

* refactoring tests

* fmt

* erc721 starting point

* fmt

* compiling

* check selectors

* create trait Erc721

* Erc721Precompile

* set mocks

* first implermentation

* first integration of erc721 in the runtime

* fmt

* check on the collection id

* test on extract owner form asset_id

* fix tests

* test for erc721 trait

* fix compilation

* refactoring

* returning address

* return value is correctly encoded as a n Address

* is_erc721_contract is not member of PrecompoileSet

* test on precompiled contracts

* erc721 returns hardcoded address

* refactoring

* testing return of asset ownership

* fix errors

* remove unused use

* refactoring

* rewriting testing mod

* refactoring

* moving check for cllection address in pallet

* collection address prefix changed

* test passing

* test passing

* refactoring tests

* do not panic the runtime

* fmt

* solidity tokenId -> _tokenId

* erc721 functions are views

* update docs

* sp-core in std , removed duplicate function

* documentaetion

* using compilator type ifer

* compiler infer the size of the array

* added to std feature

* rename variable

* trait documentation

* fmt

* runtime spec to 4

* fix name on LivingAssetsOwnership pallet (#120)

---------

Co-authored-by: dastan <[email protected]>
Co-authored-by: Dastan Samatov <[email protected]>
Co-authored-by: Toni Mateos <[email protected]>
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.

OwnerOf asset in a collection
2 participants