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

can add prev_owner/from attributes at transfer_nft and send_nft? #11

Open
ALPAC-4 opened this issue Oct 8, 2021 · 4 comments
Open

can add prev_owner/from attributes at transfer_nft and send_nft? #11

ALPAC-4 opened this issue Oct 8, 2021 · 4 comments

Comments

@ALPAC-4
Copy link

ALPAC-4 commented Oct 8, 2021

When owner approve the token and contract execute transfer, we can't know where is the token came from by the logs.

This is really hard to track token transfer. I made an example on terra.

  1. User A make approve to transfer the cw721 token to contract B
    https://finder.terra.money/columbus-5/tx/14D4251F41053E7572A740D7FDB1760A2D3FD593870B0D70FB1EDBCA7121F211

  2. User C execute the contract B to send it to B
    https://finder.terra.money/columbus-5/tx/AEA9F3EEF1AD4A29644355777BD4157422E55E50130C2D330BC6EC0F4E63F51F

The token transferred to B from A. But there is no information about that at tx of 2.

Because of this Cw20 have transfer_from and send_from.

It is possible to make the attribute that can show the previous owner?

@ALPAC-4
Copy link
Author

ALPAC-4 commented Oct 8, 2021

    fn transfer_nft(
        &self,
        deps: DepsMut,
        env: Env,
        info: MessageInfo,
        recipient: String,
        token_id: String,
    ) -> Result<Response<C>, ContractError> {
        let token = self.tokens.load(deps.storage, &token_id)?;
        let owner = token.owner;
        self._transfer_nft(deps, &env, &info, &recipient, &token_id)?;
        Ok(Response::new()
            .add_attribute("action", "transfer_nft")
            .add_attribute("sender", info.sender)
            .add_attribute("from", owner)
            .add_attribute("recipient", recipient)
            .add_attribute("token_id", token_id))
    }

maybe like this?

@larry0x
Copy link
Collaborator

larry0x commented Oct 10, 2021

Second this. Contracts receiving the NFT more likely will want to know who's the previous owner instead of who initiated the transfer.

In ERC-721 specs, the Transfer event simply contains from and to attributes. I think CW721 should do the same.

Also Transfer should really be their own events instead of lumped together with other logging info. This makes is much easier for indexers to parse transactions.

@JakeHartnell
Copy link
Collaborator

Feel free to make a PR.

@peara
Copy link
Contributor

peara commented Aug 30, 2023

@shanev @JakeHartnell should we reopen this one? I don't quite understand why previous PR was not approved.
I'd like to understand your view on this first. After that, I can open a new PR.
I want to change sender to the NFT owner, and omit the transaction sender from the event as it can always be found from the transaction data.
This could be a breaking change for some but we have versioning now and indexer can use that to determine the value.

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

Successfully merging a pull request may close this issue.

4 participants