Skip to content
This repository has been archived by the owner on Feb 4, 2022. It is now read-only.

BlockInformation Type Port from Web3.js #232

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TheGreatAxios
Copy link

  • Ported the BlockInformation from Web3.js
  • Some of the types are strings, but can be manipulated to BigInt or Uint8List potentially
  • Have not made methods for some of those potentials yet
  • Have not implemented tests yet

@simolus3 simolus3 changed the base branch from development to master December 27, 2021 13:57
Copy link
Owner

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@@ -284,7 +284,7 @@ class Web3Client {

var signed = await signTransaction(cred, transaction,
chainId: chainId, fetchChainIdFromNetworkId: fetchChainIdFromNetworkId);

print("signed: $signed");
Copy link
Owner

Choose a reason for hiding this comment

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

Probably a leftover from debugging?

Copy link
Author

Choose a reason for hiding this comment

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

Possibly, I don't remember that, but it is easily removed.

@TheGreatAxios
Copy link
Author

@simolus3 Commit #71c888a..... I just pushed up has the fields for block info set as final and the removal of that print in the client.

Let me know if you see anything else!

@codecov-commenter
Copy link

Codecov Report

Merging #232 (71c888a) into master (7d345b8) will decrease coverage by 2.39%.
The diff coverage is 32.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
- Coverage   71.42%   69.03%   -2.40%     
==========================================
  Files          53       55       +2     
  Lines        2184     2238      +54     
==========================================
- Hits         1560     1545      -15     
- Misses        624      693      +69     
Impacted Files Coverage Δ
lib/src/core/block_information.dart 0.00% <0.00%> (ø)
lib/src/core/client.dart 11.56% <0.00%> (-6.81%) ⬇️
lib/src/core/transaction_information.dart 47.56% <52.27%> (-0.55%) ⬇️
lib/src/contracts/generated_contract.dart 0.00% <0.00%> (-42.86%) ⬇️
test/infura_integration_test.dart 53.33% <0.00%> (-40.01%) ⬇️
lib/src/generated/erc20.g.dart 0.00% <0.00%> (-15.52%) ⬇️
lib/src/contracts/deployed_contract.dart 0.00% <0.00%> (-10.00%) ⬇️
lib/src/contracts/abi/abi.dart 88.88% <0.00%> (-5.56%) ⬇️
lib/src/credentials/address.dart 74.35% <0.00%> (-5.13%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d345b8...71c888a. Read the comment docs.

this.sha3Uncles,
this.size,
this.stateRoot,
this.timestamp,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can timestamp be int safely?

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah it should probably be a DateTime

final String? sha3Uncles;
final String? size;
final String? stateRoot;
final String? timestamp;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not rush to add all the info that eth_getBlockByNumber returns without carefully choosing an appropriate data type for each field. If we make them strings and then decide to change to int, it would be a breaking change.

If we do not have resources to choose carefully, we should add only the fields we are conscious about, like this timestamp which is definitely int and not null.

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

Successfully merging this pull request may close these issues.

4 participants