Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Linting and cleanup for account-integrations/safe #86

Merged
merged 9 commits into from
Sep 18, 2023

Conversation

voltrevo
Copy link
Contributor

@voltrevo voltrevo commented Sep 18, 2023

I'm finding myself doing some work in here as part of #74. As part of that, I've cleaned up the typing errors and added linting.

Highly recommended to use an IDE that can autofix most linting errors whenever you save the file. Please reach out if you have any trouble with this.

I've manually tested these changes with yarn hardhat test using the eth-infinitism bundler.

@voltrevo voltrevo changed the title Linting and cleanup Linting and cleanup for account-integrations/safe Sep 18, 2023
Comment on lines -18 to -20
let safeProxyFactory: SafeProxyFactory;
let safe: Safe;
let entryPoint: EntryPoint;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These variables fit much more neatly into the existing pattern with setupTests. Declaring uninitialized globals like this means typescript cannot know they have been initialized and puts downward pressure on typing accuracy.

@@ -95,13 +87,14 @@ describe("SafeBlsPlugin", () => {
]);

const deployedAddress = await calculateProxyAddress(
safeProxyFactory as any,
safeProxyFactory,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being able to remove as any is linked to the use of *__factory.connect(addr, runner) over ethers.getContractFactory(...).attach(addr).connect(runner) discussed elsewhere.

@@ -140,19 +133,19 @@ describe("SafeBlsPlugin", () => {
maxPriorityFeePerGas,
paymasterAndData: "0x",
signature: "",
};
} satisfies UserOperationStruct;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good case study demonstrating the utility of the new satisfies keyword. If it's unclear, I recommend this video for background, and taking a look what happens when you modify this statement back to using : UserOperationStruct.

Comment on lines -28 to -29
const factory = await hre.ethers.getContractFactory("SafeProxyFactory");
const singleton = await hre.ethers.getContractFactory("Safe");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See replacements in return statement.

Comment on lines -54 to +56
factory: factory.attach(SAFE_FACTORY_ADDRESS).connect(userWallet),
singleton: singleton.attach(SINGLETON_ADDRESS).connect(provider),
factory: SafeProxyFactory__factory.connect(
SAFE_FACTORY_ADDRESS,
userWallet,
),
singleton: Safe__factory.connect(SINGLETON_ADDRESS, provider),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This results in the same JS entities, but doing it this way results in better type information. I recommend avoiding getContractFactory in general, because the way it works is less robust than the typechain alternatives.

Copy link
Contributor

@blakecduncan blakecduncan left a comment

Choose a reason for hiding this comment

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

Thanks for adding linting 💯

# Conflicts:
#	account-integrations/safe/test/hardhat/integration/SafeWebAuthnPlugin.test.ts
@blakecduncan blakecduncan merged commit 3274a01 into main Sep 18, 2023
@blakecduncan blakecduncan deleted the linting-and-cleanup branch September 18, 2023 20:07
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.

3 participants