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

Add Address w/ checksum support to primitives #19

Merged
merged 21 commits into from
May 2, 2023
Merged

Conversation

prestwich
Copy link
Member

@prestwich prestwich commented Apr 29, 2023

  • move keccak256 to primitives
  • add Address type
  • implement checksumming and checksum parsing
  • adjust ABI types to use address instead of B160
  • update sol macro
  • remove parity hex and hash macro
  • replace with const generic fixedbytes
  • add macro for wrapped fixed bytes

closes #16

@prestwich prestwich added the enhancement New feature or request label Apr 29, 2023
@prestwich prestwich self-assigned this Apr 29, 2023
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Looks fine, but I don't understand why Address should be its own fixed-hash type when we already have B160? Can't we implement address methods on B160 and export a type alias?

primitives/src/bits/address.rs Outdated Show resolved Hide resolved
primitives/src/bits/address.rs Outdated Show resolved Hide resolved
primitives/src/bits/address.rs Outdated Show resolved Hide resolved
primitives/src/bits/hex.rs Outdated Show resolved Hide resolved
primitives/src/bits/hex.rs Outdated Show resolved Hide resolved
primitives/src/bits/address.rs Outdated Show resolved Hide resolved
primitives/src/bits/address.rs Outdated Show resolved Hide resolved
@prestwich
Copy link
Member Author

Looks fine, but I don't understand why Address should be its own fixed-hash type when we already have B160? Can't we implement address methods on B160 and export a type alias?

I've gone back and forth on this a bit, but i think that they ought to be separate. For one thing, there is a type-level distinction, as only some 20-byte hashes are Addresses

I also don't want to clutter the B160 impl space with address-specific behavior (like checksumming) or do floating util functions the way ethers currently does

@gakonst any strong opinion here?

@DaniPopes
Copy link
Member

How are not 20 byte hashes addresses? Where else is a B160 used in Ethereum other than addresses?

The u8 namespace is "cluttered" with ASCII/UTF-8 methods, u16 with UTF16 etc, I don't think this holds

@prestwich
Copy link
Member Author

prestwich commented Apr 29, 2023

How are not 20 byte hashes addresses? Where else is a B160 used in Ethereum other than addresses?

Ethereum does support the Ripemd160 hash function via precompile 3. Nobody uses it really tho and it would be returned from solidity as bytes20 which we handle as [u8;20]. I'd be amenable to dropping the B160 type entirely, and just having Address

TBH, my hot take is that it's probably not ideal to even have B type aliases. Instead just use named distinct hashes OR just [u8;N]. It's weird that we have a B256 that describes rando 32-byte arrays. Smells like a legacy parity decision. There should be a KeccakOutput and a Sha2Output 😤

The u8 namespace is "cluttered" with ASCII/UTF-8 methods, u16 with UTF16 etc,

just because type-confusion is standard in representation of strings doesn't mean we need to embrace it for our types. It should be illegal at compile-time to use EIP-55 to checksum encode a Ripemd160 output or a non-address bytes20. This is a mistake, and the type-system should prevent it from happening

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Makes sense

primitives/src/bits/address.rs Outdated Show resolved Hide resolved
primitives/src/bits/address.rs Show resolved Hide resolved
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Some methods that can panic are missing #[track_caller], while others that can't have it

primitives/src/bits/hex.rs Outdated Show resolved Hide resolved
primitives/src/bits/fixed.rs Outdated Show resolved Hide resolved
primitives/src/bits/fixed.rs Outdated Show resolved Hide resolved
primitives/src/bits/fixed.rs Outdated Show resolved Hide resolved
primitives/src/bits/fixed.rs Outdated Show resolved Hide resolved
primitives/src/bits/fixed.rs Outdated Show resolved Hide resolved
primitives/src/bits/fixed.rs Outdated Show resolved Hide resolved
primitives/src/bits/hex.rs Outdated Show resolved Hide resolved
primitives/src/bits/fixed.rs Show resolved Hide resolved
primitives/src/bits/fixed.rs Show resolved Hide resolved
@prestwich
Copy link
Member Author

this PR ended up being much larger than expected 😅

@prestwich prestwich merged commit 34643b3 into main May 2, 2023
@DaniPopes DaniPopes deleted the prestwich/address branch May 15, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Address to primitives
3 participants