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

fix(evm): L04 - Remove repeated function #829

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Conversation

chrismaree
Copy link
Member

@chrismaree chrismaree commented Jan 2, 2025

OZ identified:

The _toBytes32 [internal function](https://github.com/across-protocol/contracts/blob/401e24ccca1b3af919dd521e58acd445297b65b6/contracts/erc7683/ERC7683OrderDepositor.sol#L355) of the ERC7683OrderDepositor contract has the same logic as the imported toBytes32 [function](https://github.com/across-protocol/contracts/blob/401e24ccca1b3af919dd521e58acd445297b65b6/contracts/libraries/AddressConverters.sol#L23) of the AddressConverters library. Both the _toBytes32 and toBytes32 functions are [used](https://github.com/across-protocol/contracts/blob/401e24ccca1b3af919dd521e58acd445297b65b6/contracts/erc7683/ERC7683OrderDepositor.sol#L229-L247) throughout the ERC7683OrderDepositor contract, which lessens code clarity.

In order to improve the overall code quality and avoid having duplicated code, consider removing the _toBytes32 function and using the library function instead.

This PR addresses this as well as the same logic/problem for addresses in the same file.

Signed-off-by: Chris Maree <christopher.maree@gmail.com>
Copy link
Contributor

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chrismaree chrismaree changed the title fix: L04 - Remove repeated function fix(evm): L04 - Remove repeated function Jan 7, 2025
@chrismaree chrismaree merged commit ac8d25a into svm-dev Jan 9, 2025
9 checks passed
@chrismaree chrismaree deleted the chrismaree/l04 branch January 9, 2025 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants