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

Remove the safeSingletonContract property from FractalContracts and BaseContracts interfaces #1660

Conversation

adamgall
Copy link
Member

@adamgall adamgall commented May 6, 2024

Please review and merge #1658 first.

@adamgall adamgall self-assigned this May 6, 2024
Copy link

netlify bot commented May 6, 2024

Deploy Preview for fractal-dev ready!

Name Link
🔨 Latest commit 77d3fbe
🔍 Latest deploy log https://app.netlify.com/sites/fractal-dev/deploys/663bd6d7261c96000809712c
😎 Deploy Preview https://deploy-preview-1660.app.dev.fractalframework.xyz
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@adamgall adamgall force-pushed the remove-zodiacModuleProxyFactoryContract-from-useSafeContracts branch from 75f435e to 5ebaf5b Compare May 7, 2024 17:58
@adamgall adamgall force-pushed the remove-safeSingletonContract-from-FractalContracts-and-BaseContracts branch from 107633c to 476155d Compare May 7, 2024 17:58
Copy link
Contributor

@Da-Colon Da-Colon left a comment

Choose a reason for hiding this comment

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

Two questions and looks like github is picking up on a lint error in AzoriusTxBuilder

Comment on lines -118 to -120
if (!isHex(encodedAddOwnerWithThreshold)) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

the isHex function seems to be off and on used in conjunction with the encodeFunctionData. is there a reason its being used in some places and not others?

Copy link
Member Author

@adamgall adamgall May 7, 2024

Choose a reason for hiding this comment

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

viem's encodeFunctionData() will always return a "Hex" string, so no need to use the isHex function on data returned from that function.

It was needed when using ethers interface.encodeFunctionData(), because that function just returns a plain string, and typescript doesn't allow plain strings to be passed into viem functions that are expecting Hex.

As we progress through this refactor, we'll be removing all of these isHex checks, because we won't be using ethers any longer!

It's jsut needed temporarily for now as we have both ethers and viem in the codebase.

Comment on lines 61 to -68
bytecodeHash: keccak256(
encodePacked(
['bytes', 'uint256'],
[
safeFactoryContractProxyCreationCode,
hexToBigInt(getAddress(safeSingletonContract.address)),
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the removal of the getAddress function here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Presumably because the safeSingletonContract type went from an ethers contract object to a viem contract object.

ethers contract.address is a string
viem contract.address is an Address

@adamgall adamgall force-pushed the remove-zodiacModuleProxyFactoryContract-from-useSafeContracts branch from 5ebaf5b to 917cc5b Compare May 7, 2024 23:28
@adamgall adamgall force-pushed the remove-safeSingletonContract-from-FractalContracts-and-BaseContracts branch from 476155d to b81091e Compare May 7, 2024 23:28
@adamgall adamgall force-pushed the remove-zodiacModuleProxyFactoryContract-from-useSafeContracts branch 2 times, most recently from ccd09f8 to 3b3a125 Compare May 8, 2024 19:42
Base automatically changed from remove-zodiacModuleProxyFactoryContract-from-useSafeContracts to root/remove-typechain May 8, 2024 19:46
@adamgall adamgall force-pushed the remove-safeSingletonContract-from-FractalContracts-and-BaseContracts branch from b81091e to 77d3fbe Compare May 8, 2024 19:47
@adamgall adamgall marked this pull request as ready for review May 8, 2024 19:47
@adamgall adamgall merged commit c3bb03e into root/remove-typechain May 8, 2024
7 checks passed
@adamgall adamgall deleted the remove-safeSingletonContract-from-FractalContracts-and-BaseContracts branch May 8, 2024 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants