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

Refactor eth code: eliminate duplicate address to string conversion functions #2343

Open
dimxy opened this issue Feb 7, 2025 · 4 comments
Open
Assignees
Labels
improvement: API priority: low Tasks with low importance that can be addressed when time permits.

Comments

@dimxy
Copy link
Collaborator

dimxy commented Feb 7, 2025

Updated according to the discussion:

In eth.rs we have two public address to string conversion functions: eth_addr_to_hex() and display_eth_address().
PR #2261 also adds a trait AddrToString for the same purpose.
Also, display_eth_address() provides address format with checksum, basically for user input verification.

The required changes:

  • Let's use the trait for getting string address for internal purposes and eliminate usage of functions eth_addr_to_hex and display_eth_address (make them private if we still need them).
  • Let's use display_eth_address() to return addresses in the API result (to the GUI side). Maybe it's good to implement it as a trait.
  • Also eliminate code like: format!("{:#02x}", token_contract_address) and use the unified approach with a trait.
@dimxy dimxy added improvement: API priority: low Tasks with low importance that can be addressed when time permits. labels Feb 7, 2025
@laruh
Copy link
Member

laruh commented Feb 10, 2025

eliminate one of two functions eth_addr_to_hex and display_eth_address.
(I think display_eth_address is what we need. Also let's make it private and use as implementation for the AddrToString::addr_to_string).

We shouldn't use display_eth_address instead of eth_addr_to_hex everywhere when we only need to convert eth Address type to a string. Using display_eth_address in such cases is inefficient, as it unnecessarily computes checksum_address each time, wasting resources.

eth_addr_to_hex and display_eth_address have their own purposes:

/// `eth_addr_to_hex` converts Address to hex format.
/// Note: the result will be in lowercase.
pub fn eth_addr_to_hex(address: &Address) -> String { format!("{:#02x}", address) }
/// Checks that input is valid mixed-case checksum form address
/// The input must be 0x prefixed hex string
fn is_valid_checksum_addr(addr: &str) -> bool { addr == checksum_address(addr) }
/// `display_eth_address` converts Address to mixed-case checksum form.
#[inline]
pub fn display_eth_address(addr: &Address) -> String { checksum_address(&eth_addr_to_hex(addr)) }

So they are not duplicate conversion functions.

But it still worth making eth_addr_to_hex private and call addr_to_string (AddrToString trait method) instead.

@dimxy
Copy link
Collaborator Author

dimxy commented Feb 10, 2025

Using display_eth_address in such cases is inefficient, as it unnecessarily computes checksum_address each time, wasting resources.

Okay, let's ensure we use display_eth_address only for returned results and make this consistently (not like use both addresses with and w/o checksum in results)

BTW I can see the ethereum_types lib has implementation for 'Address' to string conversion. Let's use it maybe.

@laruh
Copy link
Member

laruh commented Feb 10, 2025

BTW I can see the ethereum_types lib has implementation for 'Address' to string conversion. Let's use it maybe.

can you provide code part or screenshot so I can find it.

@dimxy
Copy link
Collaborator Author

dimxy commented Feb 10, 2025

BTW I can see the ethereum_types lib has implementation for 'Address' to string conversion. Let's use it maybe.

can you provide code part or screenshot so I can find it.

https://github.com/paritytech/parity-common/blob/d54ac847cc5447941390b5e9e5e3baef436e1451/fixed-hash/src/hash.rs#L232

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement: API priority: low Tasks with low importance that can be addressed when time permits.
Projects
None yet
Development

No branches or pull requests

2 participants