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.

Sign up for GitHub

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

Jump to bottom

/{runtime}/transactions: Add heuristic field for "is tx a native token transfer?" #469

Merged
merged 1 commit into from
Jul 3, 2023
Merged

/{runtime}/transactions: Add heuristic field for "is tx a native token transfer?" #469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions api/spec/v1.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2303,6 +2303,16 @@ components:
type: object
description: The method call body. May be null if the transaction was malformed.
example: {"address": "t1mAPucIdVnrYBpJOcLV2nZoOFo=", "data": RBo+cAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=", "value": ""}
is_likely_native_token_transfer:
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we add this field to required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe. I do have arguments for keeping it optional, but they are relatively weak: 1) We currently just omit it if it's false. 2) It's such an unusual field (in that it's a heuristic, rather than just restructured blockchain contents) that I kinda like keeping the door ajar to removing the field later if desired.

I don't need much of a push to promote it to required later.

type: boolean
description: |
Whether this transaction likely represents a native token transfer.
This is based on a heuristic, and can change at any time without warning and possibly without updating the documentation.
The current heuristic sets this to `true` for:
- Transactions with method "accounts.Transfer". Those are always native token transfers.
- Transactions with method "evm.Call" that have no `data` field in their `body`. Those tend to be transfers, but the runtimes provides no reliable visibility into whether a transfer happened.
Note: Other transactions with method "evm.Call", and possibly "evm.Create", may also be (or include) native token transfers. The heuristic will be `false` for those.
example: true
to:
<<: *AddressType
description: |
Expand Down
20 changes: 17 additions & 3 deletions storage/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1233,16 +1233,30 @@ func (c *StorageClient) RuntimeTransactions(ctx context.Context, p apiTypes.GetR
encryptionEnvelope.Format = *encryptionEnvelopeFormat
t.EncryptionEnvelope = &encryptionEnvelope
}
// TODO: Here we render Ethereum-compatible address preimages. That's
// a little odd to do in the database layer. Move this farther out if
// we have the energy.

// Render Ethereum-compatible address preimages.
// TODO: That's a little odd to do in the database layer. Move this farther
// out if we have the energy.
if sender0PreimageContextIdentifier != nil && sender0PreimageContextVersion != nil {
t.Sender0Eth = EthChecksumAddrFromPreimage(*sender0PreimageContextIdentifier, *sender0PreimageContextVersion, sender0PreimageData)
}
if toPreimageContextIdentifier != nil && toPreimageContextVersion != nil {
t.ToEth = EthChecksumAddrFromPreimage(*toPreimageContextIdentifier, *toPreimageContextVersion, toPreimageData)
}

// Heuristically decide if this is a native runtime token transfer.
// TODO: Similarly to above, this application logic doesn't belong here (= the DB layer);
// move it out if we establish a separate app/logic layer.
if t.Method != nil {
if *t.Method == "accounts.Transfer" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

put this on the list of places to think about if a paratime ever uses multiple denominations in the accounts system

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah :/, I thought about it.
The alternative is to add the requirement that body.amount.Denomination == "", where body is the unstructured JSON column in the DB, and that's a little brittle in its own right. So I just went with the less-code option.

t.IsLikelyNativeTokenTransfer = common.Ptr(true)
} else if *t.Method == "evm.Call" && t.Body != nil && (*t.Body)["data"] == "" {
// Note: This demands that the body.data key does exist (as we expect from evm.Call tx bodies),
// but has an empty value.
t.IsLikelyNativeTokenTransfer = common.Ptr(true)
}
}

ts.Transactions = append(ts.Transactions, t)
}

Expand Down
1 change: 1 addition & 0 deletions tests/e2e_regression/expected/emerald_txs.body
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2011,6 +2011,7 @@
"gas_used": 22142,
"hash": "34204265b53f41a253f30651882bf1317739ac694ccfb966d2b57e54a82c8ee2",
"index": 3,
"is_likely_native_token_transfer": true,
"method": "evm.Call",
"nonce_0": 60,
"round": 1003586,
Expand Down