-
Notifications
You must be signed in to change notification settings - Fork 795
Conversation
ethers-core/src/utils/mod.rs
Outdated
// addr is not checksummed address | ||
true |
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.
this should be false, no?
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.
you're right, I have fixed it
ethers-core/src/utils/mod.rs
Outdated
@@ -421,6 +421,27 @@ pub fn to_checksum(addr: &Address, chain_id: Option<u8>) -> String { | |||
}) | |||
} | |||
|
|||
/// checks if the given address is a valid checksum address | |||
pub fn verify_checksum(addr: &str, chain_id: Option<u8>) -> bool { |
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.
don't love the name,
also not a fan of the to_checksum
function, should have been named to_checksum_address
but, to keep it consistent, this is fine I guess.
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.
maybe check_address_checksum
?
ref: https://github.com/web3/web3.js/blob/aaf26c8806bc9fb60cf6dcb6658104963c6c7fc7/packages/web3-utils/src/Utils.js#L140
Update ethers-core/src/utils/mod.rs Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> refactor: remove char case branch
74bcc82
to
f423d3c
Compare
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.
smol style nit
ethers-core/src/utils/mod.rs
Outdated
let address = addr.parse::<Address>(); | ||
|
||
// wrong address string | ||
if address.is_err() { | ||
return false | ||
} | ||
|
||
let checksum_addr = to_checksum(&address.unwrap(), chain_id); | ||
|
||
return checksum_addr.strip_prefix("0x").unwrap_or(&checksum_addr) == addr |
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.
use
if let Ok() {
let checksum_addr = to_checksum(&address.unwrap(), chain_id);
return checksum_addr.strip_prefix("0x").unwrap_or(&checksum_addr) == addr
}
false
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.
Done
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.
API change
ethers-core/src/utils/mod.rs
Outdated
/// checks if the given address is a valid checksum address | ||
/// | ||
/// Returns `true` if addr is a valid checksum address, `false` otherwise. | ||
pub fn verify_checksum(addr: &str, chain_id: Option<u8>) -> bool { |
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.
why return a bool instead of the address? We're doing the parsing work regardless, and the user can always call is_some
if they need a bool
basically shouldn't this be parse_checksummed
instead
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.
like this?
/// Returns a `Result<Address>`: `Ok(Address)` if addr is checksummed, `Err()` otherwise
pub fn parse_checksummed(addr: &str, chain_id: Option<u8>) -> Result<Address>
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.
function signature is awesome 👌
I'd write docs similar to this:
/// Parses an [EIP-1191](https://eips.ethereum.org/EIPS/eip-1191) checksum address. Returns `Ok(address)` if the checksummed address is valid, `Err()` otherwise. If `chain_id` is `None`, falls back to [EIP-55](https://eips.ethereum.org/EIPS/eip-55) address checksum method
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.
@prestwich refactored, pls check it
f7beeee
to
ccb6fa7
Compare
ethers-core/src/utils/mod.rs
Outdated
return Err(ConversionError::InvalidAddressChecksum) | ||
} | ||
} | ||
Err(ConversionError::TextTooLong) |
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.
rather than returning text too long, I think we can do the following
- add a hex error to the ConversionError enum
enum ConversionError {
...,
/// Hex error, usually from address parsing
#[error(transparent)]
FromHexError(#[from]hex::FromHexError)
}
- change this function to use a
?
let addr: Address = addr.strip_prefix("0x").unwrap_or(addr).parse()?
if checksum_addr.strip_prefix("0x").unwrap_or(&checksum_addr) == addr {
return Ok(address)
} else {
return Err(ConversionError::InvalidAddressChecksum)
}
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.
the FromHexError
is from rustc_hex
crate, should I add a it?
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.
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.
leave it to parity to make our lives more complicated 🤣 🙄
I think you should be able to make it work as follows:
- reference the fully qualified associated type
<Address as std::str::FromStr>::Err
in the enum variant - instead of doing a
#[from]
in the error variant, usemap_err
to convert
// variant specifies that it takes "whatever error the `parse()` implementation says is the error"
FromHexError(<Address as std::str::FromStr>::Err)
// map err on the first line of the function
let addr: Address = addr.strip_prefix("0x").unwrap_or(addr).parse().map_err(ConversionError::FromHexError)?;
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.
sorry this is a pain. if it gets too much let me know and i'll do the tidying
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.
@prestwich I've add this FromHexError
changes are good. 1 more small ergonomics tweak 🙏 |
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.
Approving, pending CI run completion. Thanks for the contribution!
Motivation
as ethers-rs provide
to_checksum
function, but there not provide a way to verify a checksum-like address.so I add a utilities function to verify a checksum address is valid.
Solution
PR Checklist