chore(other): use type aliases where possible to improve clarity #859
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jun 10, 2024
zerosnacks
commented
Jun 10, 2024
…d to be U64, not u64
mattsse
reviewed
Jun 10, 2024
zerosnacks
commented
Jun 11, 2024
zerosnacks
commented
Jun 11, 2024
zerosnacks
commented
Jun 11, 2024
mattsse
requested changes
Jun 11, 2024
I've removed the number aliases, kept the hash aliases as discussed Any existing references using the number-type aliases I've kept around, just avoided introducing any new ones. |
zerosnacks
requested review from
DaniPopes,
gakonst,
onbjerg,
prestwich and
Evalir
as code owners
mattsse
approved these changes
Jun 12, 2024
ben186
pushed a commit
to ben186/alloy
that referenced
this pull request
Jul 27, 2024
…oy-rs#859) * improve type aliasing on untyped types * prefer type alias * clean up * fix typo * fix typo * add TxNonce and BlockTimestamp * fix clippy, keep impl From as is * do not use TxNonce for EIP1186AccountProofResponse as type is expected to be U64, not u64 * avoid aliasing for genesis as it is often read from file and rarely interacted with dynamically * fix clippy * revert consensus, engine related type changes * prefer import types as long as they do not clash * fix docs link * do not use TxNonce and BlockTimestamp aliases * rule: alias for hash types, not number types * remove newly added BlockNumber references, leave the old ones as is * remove TxIndex alias use
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Reduces magic values, reduces chance of error and makes it easier for end-users to see what value they are getting e.g.
TxHashinstead ofFixedBytes<32>whenwatch()-ing a transaction. Main goal is to limit showing internal types and help with consistency / standardize fields / encourage native types (u64vsU64) and sizes.Related PR in
core: alloy-rs/core#654 + alloy-rs/core#655Solution
Proposed changes:
TxHashBlockHashu64- UseChainIdNot used:
Transaction index:u64- UseTxIndexBlock number:u64- UseBlockNumberTransaction nonce:u64- UseTxNonceBlock timestamp:u64- UseBlockTimestampShould have no impact and no logic changes should be present.
From testing it appears that
rust-analyzerdoes not yet correctly deal with type aliases (long standing issue), as in it forwards the rust-lang/rust-analyzer#1666Fortunately it does appear to be supported in RustRover:
PR also includes a number of QOL fixes
PR Checklist