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: emit event for refund in ft_resolve_transfer() #752

Merged
merged 4 commits into from
Mar 16, 2022

Conversation

think-in-universe
Copy link
Member

@think-in-universe think-in-universe commented Mar 11, 2022

In the standard fungible_token implementation, we have emitted FtTransfer in ft_transfer(), but no events was emitted for refund in ft_resolve_transfer(), which may introduce inconsistency if aggregating the data via FT events such as total deposits and withdrawals.

To fix this issue, we can either reuse the current FtTransfer, FtBurn events or create a new FtRefund event if needed. In this PR, we reuse the existing events.

Verified

This commit was signed with the committer’s verified signature.
TheSpiritXIII Daniel Hrabovcak
Copy link
Contributor

@itegulov itegulov left a comment

Choose a reason for hiding this comment

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

Yeah, this makes sense to me. Missed this in #723.

FYI you need to regenerate the examples' wasm files by running ./examples/build_all_docker.sh in the project directory, otherwise the CI won't pass.

@think-in-universe
Copy link
Member Author

@itegulov thanks. updated.

@austinabell
Copy link
Contributor

I believe the rationale for leaving out in nft (and then what propagated to this) was that since these were optional (iirc) they should not be included in the standard. @telezhnaya @frol I don't have the context to know if this should change, could you weigh in when you get the chance?

If we change this, the NFT events should probably as well for consistency

@itegulov
Copy link
Contributor

I believe the rationale for leaving out in nft (and then what propagated to this) was that since these were optional (iirc) they should not be included in the standard.

Explicit minting and burning are indeed optional, but ft_resolve_transfer is not. And since using ft_resolve_transfer can result in some tokens being burnt, I think it makes sense to emit an event.

If we change this, the NFT events should probably as well for consistency

I have just checked NFT events, and we are already emitting a transfer event when returning an NFT to its previous owner: https://github.com/near/near-sdk-rs/blob/master/near-contract-standards/src/non_fungible_token/core/core_impl.rs#L526. And since NFT standard does not require storage deposits from users, it never has to burn NFTs by itself, so the flow there is a bit different.

@telezhnaya
Copy link
Contributor

If we send the token back, we should emit the event about it, otherwise, the data will be inconsistent. I agree with the proposed solution.

I was also thinking about emitting the event only after we have the result of ft_resolve_transfer(). If the transfer failed, we could emit no event about it at all. But, it's better to show such unsuccessful attempts in the Wallet history, so I suggest using the current solution.

@austinabell
Copy link
Contributor

If we send the token back, we should emit the event about it, otherwise, the data will be inconsistent. I agree with the proposed solution.

I was also thinking about emitting the event only after we have the result of ft_resolve_transfer(). If the transfer failed, we could emit no event about it at all. But, it's better to show such unsuccessful attempts in the Wallet history, so I suggest using the current solution.

Why is there this inconsistency with nft vs ft standard for the burn event? The same pattern happens with the burn in NFT

// The token was burned and doesn't exist anymore.
and it was suggested (#717 (comment)) that only transfer be included in NFT standard.

@austinabell austinabell merged commit 8d7fcb2 into master Mar 16, 2022
@austinabell austinabell deleted the feat/ft-refund-event branch March 16, 2022 21:01
@frol
Copy link
Collaborator

frol commented Mar 17, 2022

Why is there this inconsistency with nft vs ft standard for the burn event?

@austinabell Olga just wanted to confirm that without this fix there will, indeed, be inconsistency, and this fix is correct.

@austinabell
Copy link
Contributor

Why is there this inconsistency with nft vs ft standard for the burn event?

@austinabell Olga just wanted to confirm that without this fix there will, indeed, be inconsistency, and this fix is correct.

Reading over the logic again seems like my intuitions were incorrect. Seems the path I linked was handling the case where the token was burned already, so no need to emit the event here. I believe no other change is needed.

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.

None yet

5 participants