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

implementation of internal transaction exporter #104

Merged
merged 3 commits into from Oct 11, 2018
Merged

implementation of internal transaction exporter #104

merged 3 commits into from Oct 11, 2018

Conversation

evgeniuz
Copy link
Contributor

@evgeniuz evgeniuz commented Oct 7, 2018

Fixes #53

@evgeniuz
Copy link
Contributor Author

evgeniuz commented Oct 7, 2018

I've based fields for export on #72:

block_number
block_timestamp
hash
from_address
to_address
value
contract_address
input
transaction_type
gas
gas_used
trace_id
is_error
err_code

But have couple of questions:

  1. block_timestamp requires additional call to getBlock for each block, as trace_filter does not return it. Should I remove this field?
  2. contract_address should contain "Address of the contract which initiated the transaction", but in the context of internal transaction that is always equals from address, as this is address that initiated transaction. What is the purpose of having separate field?
  3. call (and it's subtypes) and create transactions are fitting to these fields (with small adjustments: create is missing to and input, but we can use zero address and init respectively as fair substitutes). But suicide transactions are special, they miss lots of fields (i. e. from, to, value, input, gas, gas_used). And have special fields like balance, address (of destroyed contract) and refundAddress. Should I store them in same file anyway or should I make separate output for suicide calls?
  4. Should this job only export internal transactions (initiated by contracts) or normal ones as well?

My next steps is clean up code a bit and start adding unit tests (in addition to any comments and suggestions).

@medvedev1088
Copy link
Member

medvedev1088 commented Oct 8, 2018

@evgeniuz great job!

block_timestamp requires additional call to getBlock for each block, as trace_filter does not return it. Should I remove this field?

Yes let's remove it.

contract_address should contain "Address of the contract which initiated the transaction", but in the context of internal transaction that is always equals from address, as this is address that initiated transaction. What is the purpose of having separate field?

I think the description of the field is wrong there. It should be "The address of the created contract for 'create' trace type". It should be in the result.address field in the returned traces. You can also refer here https://github.com/analyseether/ether_sql/blob/master/ether_sql/models/traces.py#L122.

call (and it's subtypes) and create transactions are fitting to these fields (with small adjustments: create is missing to and input, but we can use zero address and init respectively as fair substitutes). But suicide transactions are special, they miss lots of fields (i. e. from, to, value, input, gas, gas_used). And have special fields like balance, address (of destroyed contract) and refundAddress. Should I store them in same file anyway or should I make separate output for suicide calls?

Let's keep them in the same file and map the fields the same way as it's done here for simplicity https://github.com/analyseether/ether_sql/blob/master/ether_sql/models/traces.py#L136. I.e
address -> from_address,
refundAddress -> to_address,
balance -> value.

Should this job only export internal transactions (initiated by contracts) or normal ones as well?

Let's include them all. Let's also not skip traces with type 'reward'. This way it will be less surprising for users and they can simply refer to parity docs if they want to know what's included in the table.

Could you also add these fields:

transaction_hash - hash of the transaction
subtraces - the number of subtraces

Rename transaction_type to trace_type (transaction_type may be confusingly understood as referring to type of transaction instead of internal transaction) and remove is_error field (seems redundant).

I'm also thinking about renaming internal_transactions to traces for consistency with parity and geth but it doesn't have to be in the scope of this task (I can create a separate task).

Also when you create mock responses for tests, please use the real parity responses from mainnet. This way the same tests can be run against parity node.

Thanks!

@evgeniuz
Copy link
Contributor Author

evgeniuz commented Oct 9, 2018

Renamed internal_transactions to traces, improved fields as suggested, added tests with fixtures based on mainnet responses. Reward traces are not skipped now, they don't have a lot of fields (even transaction_hash, but I've mapped to_address to block author (since that's where value goes, this seems to make sense).

@evgeniuz evgeniuz changed the title WIP: implementation of internal transaction exporter implementation of internal transaction exporter Oct 9, 2018
@evgeniuz evgeniuz changed the base branch from master to develop October 10, 2018 20:07
@evgeniuz
Copy link
Contributor Author

rebased on top of develop branch

@medvedev1088 medvedev1088 merged commit 9b31592 into blockchain-etl:develop Oct 11, 2018
@rraina
Copy link

rraina commented Oct 16, 2018

Nice job @evgeniuz !

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

3 participants