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

implemented getTransactionReceiptByHash #99

Merged
merged 3 commits into from
Nov 8, 2021
Merged

Conversation

shunjizhan
Copy link
Collaborator

@shunjizhan shunjizhan commented Nov 8, 2021

Change

  • added config file and command to auto generate types from graphql schema
  • added helper function to provide query functions to interact with evm-subql
  • implemented getTransactionReceiptByHash function which uses the above helper function to fetch data from evm-subql
  • updated README

Test

  • for query helpers, added a couple unit tests
  • for eth-rpc-adaptor, tested a couple GET queries to :8545 and they returned correct result.

[Change]
- added config file to auto generate types from graphql schema
- added helper function to provide query functions to interact with evm-subql
- implemented getTransactionReceiptByHash function which uses the above helper function to fetch data from evm-subql

[Test]
- for query helpers, added a couple unit tests
- for eth-rpc-adaptor, ran it and tested a couple queries to make sure they return correct result.
@shunjizhan shunjizhan self-assigned this Nov 8, 2021
@shunjizhan shunjizhan requested review from ntduan and xlc November 8, 2021 05:10
@shunjizhan shunjizhan changed the title Implemented getTransactionReceiptByHash implemented getTransactionReceiptByHash Nov 8, 2021
// TODO: correct values of these?
const confirmations = 0;
const effectiveGasPrice = BIGNUMBER_ONE;
const byzantium: false = false;
Copy link
Member

Choose a reason for hiding this comment

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

return {
to: tx.to || defaultAddress,
from: tx.from,
contractAddress: tx.contractAddress || defaultAddress,
Copy link
Member

Choose a reason for hiding this comment

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

according to https://infura.io/docs/ethereum/json-rpc/eth-getTransactionReceipt it should be null if no contractAddress

the contract address created, if the transaction was a contract creation, otherwise - null.

const defaultAddress = '0x';

return {
to: tx.to || defaultAddress,
Copy link
Member

Choose a reason for hiding this comment

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

according to https://infura.io/docs/ethereum/json-rpc/eth-getTransactionReceipt it should be null if no to

address of the receiver. Null when the transaction is a contract creation transaction.

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 but this would conflict with the type library we use:
https://github.com/ethers-io/ethers.js/blob/b1458989761c11bf626591706aa4ce98dae2d6a9/packages/abstract-provider/src.ts/index.ts#L106
according to it, to and contractAddress should both the string, also confirmations, effectiveGasPrice, byzantium are required.

Is there a more accurate lib that reflect EVM types? Or should we overwrite @etherproject to make the type match infura?

Copy link
Collaborator Author

@shunjizhan shunjizhan Nov 8, 2021

Choose a reason for hiding this comment

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

maybe we should use the parity frontier lib as our type definition? Is there a TS lib with it?🤔

Copy link
Collaborator Author

@shunjizhan shunjizhan Nov 8, 2021

Choose a reason for hiding this comment

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

The infura definition seems to be the same as eth wiki, but don't know why @ethersproject definition is so different from them... Opened a discussion there but no one answered
ethers-io/ethers.js#2270

Copy link
Member

Choose a reason for hiding this comment

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

ok. I am ok with the current code until something else complains it needs to be null.
can you write an issue and we can merge this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sounds good! Created a new issue #100 to address this

@shunjizhan shunjizhan merged commit 5e98715 into master Nov 8, 2021
@shunjizhan shunjizhan deleted the add-subql-evm-query branch November 8, 2021 10:02
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

2 participants