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

Use owner event to populate ERC721 transfer topic #1932

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

codchen
Copy link
Collaborator

@codchen codchen commented Nov 14, 2024

Describe your changes and provide context

PR sei-protocol/sei-wasmd#67 introduces a new event type indicating the pre-transfer owner of a CW721 token. This PR leverages the new event type to populate the owner topic of synthetic ERC721 transfer event correctly (previously it's set to the sender, which may be the spender and not the actual owner, and is against ERC721 convention).

Since events and receipts are not part of consensus state, this PR shouldn't be breaking app hash.

Testing performed to validate your change

unit test

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 76.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 61.62%. Comparing base (e8e4b3b) to head (72f99e5).
Report is 208 commits behind head on main.

Files with missing lines Patch % Lines
app/receipt.go 76.00% 3 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1932      +/-   ##
==========================================
- Coverage   61.64%   61.62%   -0.03%     
==========================================
  Files         365      263     -102     
  Lines       26178    23395    -2783     
==========================================
- Hits        16138    14417    -1721     
+ Misses       8967     7973     -994     
+ Partials     1073     1005      -68     
Files with missing lines Coverage Δ
app/receipt.go 79.15% <76.00%> (ø)

... and 217 files with indirect coverage changes

---- 🚨 Try these New Features:

@@ -185,9 +185,32 @@ func (app *App) translateCW721Event(ctx sdk.Context, wasmEvent abci.Event, point
if tokenID == nil {
return nil, false
}
sender := app.GetEvmAddressAttribute(ctx, wasmEvent, "sender")
ownerEvents := GetEventsOfType(response, wasmtypes.EventTypeCW721PreTransferOwner)
for _, ownerEvent := range ownerEvents {
Copy link
Contributor

Choose a reason for hiding this comment

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

might be good to add a comment here re. why we're doing this - something like
"ERC721 standards dictate that the "sender" in the logs are the actual owner, and in cases where the owner is not the caller, we look for a matching token_id and extract the owner from synthetic events"

require.Equal(t, common.HexToHash("0x0").Bytes(), receipt.Logs[0].Data)

// burn
payload = []byte("{\"burn\":{\"token_id\":\"2\"}}")
// revoke all
Copy link
Contributor

Choose a reason for hiding this comment

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

do we also test a transfer on behalf of someone else?

}
ownerAcc, err := sdk.AccAddressFromBech32(string(ownerEvent.Attributes[2].Value))
if err != nil {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

should we log the error? (albeit rare/unlikely)

@codchen codchen force-pushed the use-owner-as-event-sender branch from 596b6b9 to 99df387 Compare November 15, 2024 03:02
app/receipt.go Fixed Show fixed Hide fixed
app/receipt.go Fixed Show fixed Hide fixed
@codchen codchen force-pushed the use-owner-as-event-sender branch from 99df387 to 2b69774 Compare November 20, 2024 05:12
@codchen codchen force-pushed the use-owner-as-event-sender branch from 2b69774 to 72f99e5 Compare November 20, 2024 06:35
@codchen codchen merged commit dddd24b into main Nov 20, 2024
49 checks passed
@codchen codchen deleted the use-owner-as-event-sender branch November 20, 2024 08:30
@dvli2007
Copy link
Contributor

dvli2007 commented Nov 20, 2024

@codchen @stevenlanders @philipsu522 For CW721s, there is no "owner" attribute when a burn happens. You still will need to grab the "sender" attribute. I actually recommend you guys pull out the "burn" event mapping into its own case.

Edit: Nevermind. I see that wasmd is being modified to add an "owner" event for transfer_nft, send_nft, and burn actions.

Example of a burn: https://www.seiscan.app/pacific-1/txs/E95774B632C12AA9ABA5D5ACC36999C0EBD36612A8AC7C63199DCEB2993359FF~

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

Successfully merging this pull request may close these issues.

4 participants