-
Notifications
You must be signed in to change notification settings - Fork 274
BlockInformation Type Port from Web3.js #232
base: master
Are you sure you want to change the base?
Conversation
TheGreatAxios
commented
Dec 26, 2021
- 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍
lib/src/core/client.dart
Outdated
@@ -284,7 +284,7 @@ class Web3Client { | |||
|
|||
var signed = await signTransaction(cred, transaction, | |||
chainId: chainId, fetchChainIdFromNetworkId: fetchChainIdFromNetworkId); | |||
|
|||
print("signed: $signed"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 Report
@@ 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
Continue to review full report at Codecov.
|
this.sha3Uncles, | ||
this.size, | ||
this.stateRoot, | ||
this.timestamp, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.