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

ERC721.transferFrom #90

Closed
tonimateos opened this issue Jul 27, 2023 · 3 comments · Fixed by #129, #137, #145 or #146
Closed

ERC721.transferFrom #90

tonimateos opened this issue Jul 27, 2023 · 3 comments · Fixed by #129, #137, #145 or #146
Assignees

Comments

@tonimateos
Copy link
Contributor

tonimateos commented Jul 27, 2023

As an asset owner, I want to be able to execute ERC721.transferFrom, so I can transfer assets

Link to ERC721 standard, which specifies rules that must be checked by transferFrom - here

ACCEPTANCE

  • I can execute contractAddress.transferFrom in a manner that fails unless the conditions defined by the ERC721 standard are met
  • After successful execution, the method ownerOf returns the new owner.
  • In this task: implemented as only non-payable, so if someone transfers money in the call, it reverts.
@tonimateos tonimateos added this to LAOS Jul 26, 2023
@tonimateos tonimateos converted this from a draft issue Jul 27, 2023
@tonimateos tonimateos added this to the Deliverable 2.1 milestone Jul 27, 2023
@tonimateos tonimateos removed the status in LAOS Aug 8, 2023
@tonimateos tonimateos changed the title Transfer of asset via ERC721.transferFrom ERC721.transferFrom Aug 10, 2023
@tonimateos tonimateos moved this to Todo in LAOS Aug 10, 2023
@magecnion magecnion self-assigned this Aug 16, 2023
@magecnion magecnion moved this from Todo to In Progress in LAOS Aug 16, 2023
@magecnion magecnion linked a pull request Aug 22, 2023 that will close this issue
@magecnion magecnion moved this from In Progress to Review in LAOS Aug 22, 2023
@magecnion magecnion removed their assignment Aug 22, 2023
@magecnion magecnion moved this from Review to In Progress in LAOS Aug 23, 2023
@magecnion magecnion self-assigned this Aug 23, 2023
@magecnion magecnion moved this from In Progress to Todo in LAOS Aug 23, 2023
@magecnion magecnion moved this from Todo to In Progress in LAOS Aug 24, 2023
@asiniscalchi asiniscalchi moved this from In Progress to QA in LAOS Aug 25, 2023
@asiniscalchi asiniscalchi self-assigned this Aug 25, 2023
@asiniscalchi asiniscalchi moved this from QA to Done in LAOS Aug 25, 2023
@magecnion magecnion moved this from Done to In Progress in LAOS Aug 25, 2023
@magecnion magecnion linked a pull request Aug 29, 2023 that will close this issue
@magecnion magecnion assigned magecnion and unassigned magecnion Aug 29, 2023
@magecnion magecnion reopened this Aug 29, 2023
@magecnion
Copy link
Contributor

Considerations and Thoughts Related to the Task

  • transferFrom is only accessible when the action is performed through the precompiles module.
  • When querying assetOwner from the runtime storage using, for example, the Polkadot.js client, the default initial owner logic is not applied because it is also part of the precompiles.
  • The owner account_id used to store the mapping asset_id => owner in the runtime is the same as H160 (20 bytes), but it is extended to 32 bytes with zeros on the left. This means the account cannot be used to perform authenticated actions (e.g., we cannot verify that an extrinsic has been signed by such an account). We need to implement something similar to Moonbeam's unified accounts Moonbeam Unified Accounts.

@ghost
Copy link

ghost commented Aug 30, 2023

Considerations and Thoughts Related to the Task

  • transferFrom is only accessible when the action is performed through the precompiles module.
  • When querying assetOwner from the runtime storage using, for example, the Polkadot.js client, the default initial owner logic is not applied because it is also part of the precompiles.
  • The owner account_id used to store the mapping asset_id => owner in the runtime is the same as H160 (20 bytes), but it is extended to 32 bytes with zeros on the left. This means the account cannot be used to perform authenticated actions (e.g., we cannot verify that an extrinsic has been signed by such an account). We need to implement something similar to Moonbeam's unified accounts Moonbeam Unified Accounts.
  1. yes, I think it's fine. We don't expose any extrinsic that can perform the transfer, so we should be safe on that regard as well.
  2. correct. As I mentioned in the channel, one way to approach it is to have an RPC that can return the "correct" ownerOf by applying the logic that we apply in the precompiles. And this will be interface for Substrate clients
  3. yes, exactly. I also think that we should have unified accounts.

@magecnion magecnion moved this from In Progress to QA in LAOS Aug 31, 2023
@magecnion magecnion removed their assignment Aug 31, 2023
@magecnion magecnion linked a pull request Aug 31, 2023 that will close this issue
@tonimateos
Copy link
Contributor Author

QA: The logic works well (you can transfer only if you're the owner, the owner changes on transfer, etc), but the emitted Transfer event seems to contains a "tokenId" not related to the transferred tokenId

Image

@tonimateos tonimateos moved this from QA to In Progress in LAOS Aug 31, 2023
@tonimateos tonimateos linked a pull request Aug 31, 2023 that will close this issue
@asiniscalchi asiniscalchi moved this from In Progress to QA in LAOS Aug 31, 2023
@magecnion magecnion removed their assignment Aug 31, 2023
@tonimateos tonimateos self-assigned this Aug 31, 2023
@tonimateos tonimateos moved this from QA to Done in LAOS Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.