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

Optimize to_checksum_address function #269

Merged
merged 2 commits into from
Jun 16, 2022
Merged

Conversation

Uxio0
Copy link
Member

@Uxio0 Uxio0 commented Jun 15, 2022

While developing Safe Tx Service we found out that a lot of time was spent on converting addresses from bytes to ChecksumAddress. Biggest part of this time was overhead added on eth_utils library to keccak256 function.

So we decided to use pysha3 library (way faster than pycryptodome) directly. That's why we implemented new methods:

  • fast_keccak
  • fast_keccak_hex
  • fast_to_checksum_address
  • fast_bytes_to_checksum_address
  • fast_is_checksum_address

This is also very relevant for the EthereumAddressV2Field, as it stores addresses as bytes on database and returns them as ChecksumAddress

Related to:

@coveralls
Copy link

coveralls commented Jun 15, 2022

Pull Request Test Coverage Report for Build 2508862086

  • 105 of 107 (98.13%) changed or added relevant lines in 15 files are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.1%) to 90.578%

Changes Missing Coverage Covered Lines Changed/Added Lines %
gnosis/eth/utils.py 47 49 95.92%
Files with Coverage Reduction New Missed Lines %
gnosis/eth/django/models.py 2 85.42%
gnosis/safe/safe.py 2 87.58%
Totals Coverage Status
Change from base Build 2508621983: -0.1%
Covered Lines: 2932
Relevant Lines: 3237

💛 - Coveralls

@Uxio0 Uxio0 force-pushed the optimize-checksum-calculation branch from 4641772 to c6b498a Compare June 15, 2022 16:06
@Uxio0
Copy link
Member Author

Uxio0 commented Jun 16, 2022

Related to safe-global/safe-pm#89

Comment on lines +46 to +51
norm_address[i].upper()
if int(address_hash[i], 16) > 7
else norm_address[i]
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, done @mikhailxyz

@Uxio0 Uxio0 force-pushed the optimize-checksum-calculation branch from c6b498a to 1578363 Compare June 16, 2022 11:13
@@ -888,9 +892,9 @@ def retrieve_fallback_handler(
self.address,
self.FALLBACK_HANDLER_STORAGE_SLOT,
block_identifier=block_identifier,
)[-20:]
)[-20:].rjust(20, b"\0")
Copy link
Member

Choose a reason for hiding this comment

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

Why needs left padding?

Copy link
Member Author

@Uxio0 Uxio0 Jun 16, 2022

Choose a reason for hiding this comment

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

Because get_storage ignores leading zeroes, I don't really know why

@Uxio0 Uxio0 force-pushed the optimize-checksum-calculation branch 2 times, most recently from 061ded5 to 2ac4059 Compare June 16, 2022 11:53
@Uxio0 Uxio0 requested review from mmv08 and moisses89 June 16, 2022 11:53
While developing Safe Tx Service we found out that a lot of time was spent
on converting addresses from `bytes` to `ChecksumAddress`. Biggest part
of this time was overhead added on `eth_utils` library to `keccak256`
function.

So we decided to use `pysha3` library (way faster than `pycryptodome`)
directly. That's why we implemented new methods:

- fast_keccak
- fast_keccak_hex
- fast_keccak_hex
- fast_to_checksum_address
- fast_bytes_to_checksum_address
- fast_is_checksum_address

This is also very relevant for the `EthereumAddressV2Field`, as it
stores addresses as `bytes` on database and returns them as
`ChecksumAddress`

Related to:
  - ethereum/eth-utils#95
  - ethereum/eth-hash#35
@Uxio0 Uxio0 force-pushed the optimize-checksum-calculation branch from 2ac4059 to c3f2f15 Compare June 16, 2022 12:01
@Uxio0 Uxio0 merged commit d0915a2 into master Jun 16, 2022
@Uxio0 Uxio0 deleted the optimize-checksum-calculation branch June 16, 2022 13:44
@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants