zksync-sdk / zksync2-js

MIT License
25 stars 29 forks source link

refactor: optimize the code to improve scalability and readability. #11

Closed ylmin closed 10 months ago

ylmin commented 10 months ago

What :computer:

Why :hand:

Evidence :camera:

image
danijelTxFusion commented 10 months ago

@ylmin

Thank you for your contribution to zksync2-js. I am glad that you have provided suggestions to refactor the code to enhance its clarity. Please note that some of the suggestions are slightly outdated, as a new version, 0.2.0-beta.0, has been released. This release includes updates to ContractFactory.encodeCalldata and ContractFactory.getDeploymentTransaction.

The proposal for handling the salt has been fixed.

Regarding the encodeCalldata() function, I appreciate the effort to improve its readability. It's important to remember that code readability and clarity can be subjective and vary from person to person. In your proposal, it may not be immediately clear how the solution is more scalable than the original implementation. While the if/else statements have been replaced with an object, it still depends on whether you want to add a new property to the object or an else-if statement. This is a matter of personal preference, but it does offer an opportunity to reconsider the refactoring of this piece of code to make it even clearer. You can find the current implementation here

const contractDeploymentArgs = [salt, bytecodeHash, constructorCalldata];
const accountDeploymentArgs = [...contractDeploymentArgs, AccountAbstractionVersion.Version1];
if (this.deploymentType === "create") {
        return CONTRACT_DEPLOYER.encodeFunctionData("create", [...contractDeploymentArgs]);
 } else if (this.deploymentType === "createAccount") {
        return CONTRACT_DEPLOYER.encodeFunctionData("createAccount", [...accountDeploymentArgs]);
 } else if (this.deploymentType === "create2") {
         return CONTRACT_DEPLOYER.encodeFunctionData("create2", [...contractDeploymentArgs]);
 } else if (this.deploymentType === "create2Account") {
          return CONTRACT_DEPLOYER.encodeFunctionData("create2Account", [...accountDeploymentArgs]);
 } else {
           throw new Error(`Unsupported deployment type ${this.deploymentType}`);
 }