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

feat: added prev_owner to response log #12

Closed

Conversation

edison0xyz
Copy link

Resolves #11

Useful feature and have added it in.

@@ -194,6 +194,7 @@ fn transferring_nft() {
Response::new()
.add_attribute("action", "transfer_nft")
.add_attribute("sender", "venus")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Almost like @larry0x's comment of using from and to like the Ethereum transfer events.

It is important to maintain sender, as that may be different than the owner, even though there is a little duplication there.

Copy link
Collaborator

@JakeHartnell JakeHartnell left a comment

Choose a reason for hiding this comment

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

TransferNFT needs to be consistent with SendNFT.

@JakeHartnell
Copy link
Collaborator

What do folks think about using to over recipient? Mirroring Ethereum: https://ethereum.org/en/developers/docs/standards/tokens/erc-721/#events

@the-frey
Copy link
Collaborator

FWIW I'm in favour of sticking close to the ETH standards where possible. from and to are probably the way to go in that case IIUC.

@iLoveBusinessDevelopment
Copy link
Collaborator

I think we should continue with recipient. CW20 is already widely used at this point and uses recipient.

@the-frey
Copy link
Collaborator

Hmmm, I suppose I'm looking at the POV of devs who are coming from ETH/ERC721 land. Although can see the argument for CW20 style too 🤷

@edison0xyz
Copy link
Author

I think that we might be better to stick with the current CW standards.
Though i see the point of standardising with the Ethereum contracts, but there should be consistency amongst the CosmWasm contracts.

Changing everything to Ethereum sounds like a lot of work in code-change and communication with exisiting protocols. So perhaps let's avoid that.

Copy link
Contributor

@orkunkl orkunkl left a comment

Choose a reason for hiding this comment

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

LGTM

@ALPAC-4
Copy link

ALPAC-4 commented Oct 14, 2021

And I think we need to add owner at mint response log

https://github.com/CosmWasm/cw-nfts/blob/2712c05fbc365a44809c904f8508410c4f70383e/contracts/cw721-base/src/execute.rs#L111

I think owner is more important information than minter. Minter is always same for same contract

@shanev
Copy link
Member

shanev commented Apr 21, 2022

This issue is a bit stale. Okay to close?

@JakeHartnell
Copy link
Collaborator

This issue is a bit stale. Okay to close?

Yeah.

@peara
Copy link
Contributor

peara commented Sep 5, 2023

@JakeHartnell @larry0x @shanev Please reconsider this PR. This is very useful for indexers and apps and for the consistency of cw721 as well.
I think adding owner as already implemented is good enough. The only issue remaining is adding the same to send_nft and mint, is that right?

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 this pull request may close these issues.

can add prev_owner/from attributes at transfer_nft and send_nft?
8 participants