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

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

Merged
merged 1 commit into from Jul 3, 2023

Conversation

mitjat
Copy link
Collaborator

@mitjat mitjat commented Jun 29, 2023

Resolves https://app.clickup.com/t/866ahv3h0

We cannot know if an evm.Call tx transfers native runtime tokens or not, but we can expose a heuristic that all clients will consume.

This PR adds an is_likely_native_token_transfer field to the runtime tx output, with that heuristic.

Copy link
Contributor

@csillag csillag left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this makes sense. (But someone with more GO experience should also review this.)

@@ -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.

// 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.

Copy link
Collaborator

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

description is very detailed. could fall out of sync with implementation. please keep an eye out for changes to the heuristic

@mitjat
Copy link
Collaborator Author

mitjat commented Jul 3, 2023

description is very detailed. could fall out of sync with implementation.

I agree. The description does say "This is based on a heuristic, and can change at any time without warning and possibly without updating the documentation."
I added the detailed description because I feel it also give a bit of a rationale for why this field is even desirable (i.e. evm.Call txs with their murky semantics)

@mitjat mitjat force-pushed the mitjat/evm-expose-is-transfer-heuristic branch from 28459e8 to 2603cfc Compare July 3, 2023 05:33
@mitjat mitjat enabled auto-merge July 3, 2023 05:33
@mitjat mitjat merged commit 774ab49 into main Jul 3, 2023
6 checks passed
@mitjat mitjat deleted the mitjat/evm-expose-is-transfer-heuristic branch July 3, 2023 05:40
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

4 participants