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

Update Dependencies #27

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Update Dependencies #27

merged 1 commit into from
Aug 14, 2024

Conversation

bh2smith
Copy link
Collaborator

@bh2smith bh2smith commented Aug 14, 2024

PR Type

enhancement, dependencies


Description

  • Updated the default value for mpcContractId option in CLI.
  • Simplified the signature retrieval logic in getNearSignature by removing the loop for multiple signatures.
  • Replaced MultichainContract with MpcContract in TransactionManager and made mpcContractId a required parameter.
  • Made mpcContractId a required field in UserOptions and reordered fields.
  • Updated the sample environment variable for NEAR contract in .env.sample.
  • Updated near-ca dependency version and several development dependencies in package.json.
  • Added start script and removed ex from all script in package.json.

Changes walkthrough 📝

Relevant files
Enhancement
cli.ts
Update default `mpcContractId` in CLI options                       

examples/cli.ts

  • Updated the default value for mpcContractId option.
+1/-1     
near.ts
Simplify signature retrieval logic in `getNearSignature` 

src/lib/near.ts

  • Simplified the signature retrieval logic.
  • Removed loop for multiple signatures.
  • +6/-8     
    tx-manager.ts
    Replace `MultichainContract` with `MpcContract` and update parameters

    src/tx-manager.ts

  • Replaced MultichainContract with MpcContract.
  • Made mpcContractId a required parameter.
  • +3/-6     
    types.ts
    Update `UserOptions` to make `mpcContractId` required       

    src/types.ts

  • Made mpcContractId a required field in UserOptions.
  • Reordered fields in UserOptions.
  • +2/-2     
    Configuration changes
    .env.sample
    Update sample environment variable for NEAR contract         

    .env.sample

    • Updated sample environment variable for NEAR contract.
    +1/-1     
    Dependencies
    package.json
    Update dependencies and scripts in package.json                   

    package.json

  • Updated near-ca dependency version.
  • Updated several development dependencies.
  • Added start script.
  • Removed ex from all script.
  • +6/-5     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @bh2smith bh2smith merged commit 6d96059 into main Aug 14, 2024
    1 check passed
    @bh2smith bh2smith deleted the mpc-update branch August 14, 2024 08:11
    @mintbase-codium-pr-agent
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Logic Change
    The logic in getNearSignature was changed from handling multiple signatures to only one. Ensure this change aligns with expected behavior and that all scenarios are still correctly handled.

    Dependency Change
    Replaced MultichainContract with MpcContract and made mpcContractId a required parameter. Verify that MpcContract correctly handles all functionalities previously covered by MultichainContract.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Include a test command in the all script to ensure code integrity before deployment

    Update the all script to include a test command to ensure all changes are verified
    before deployment.

    package.json [17]

    -"all": "yarn fmt && yarn lint && yarn build"
    +"all": "yarn fmt && yarn lint && yarn build && yarn test"
     
    Suggestion importance[1-10]: 10

    Why: Including a test command in the all script is a best practice to ensure that all changes are verified and tested before deployment, enhancing code quality and reliability.

    10
    Add validation for the default contract ID to ensure it meets expected criteria

    Consider validating the default value for mpcContractId to ensure it matches
    expected formats or network conditions, especially since it's a critical
    configuration for network interactions.

    examples/cli.ts [26]

    -default: "v1.signer-prod.testnet",
    +default: validateContractId("v1.signer-prod.testnet"),
     
    Suggestion importance[1-10]: 8

    Why: Adding validation for the default contract ID is a good practice to ensure that the configuration is correct and prevents potential runtime errors due to incorrect values.

    8
    Possible issue
    Handle multiple signatures to accommodate various signing scenarios

    Replace the single signature retrieval with a loop to handle multiple possible
    signatures, which aligns with the original design for handling multiple outcomes
    from the sign method.

    src/lib/near.ts [10-15]

    -const signature = await adapter.sign(viemHash);
    -if (
    -  ethers.recoverAddress(hash, signature).toLocaleLowerCase() ===
    -  adapter.address.toLocaleLowerCase()
    -) {
    -  return signature;
    +const signatures = await adapter.sign(viemHash);
    +for (const signature of signatures) {
    +  if (
    +    ethers.recoverAddress(hash, signature).toLocaleLowerCase() ===
    +    adapter.address.toLocaleLowerCase()
    +  ) {
    +    return signature;
    +  }
     }
     
    Suggestion importance[1-10]: 9

    Why: Reverting to handling multiple signatures aligns with the original design and ensures robustness in different signing scenarios, which is crucial for security and correctness.

    9
    Maintainability
    Make mpcContractId optional in the configuration for better flexibility

    Ensure that mpcContractId is optional in the configuration to maintain backward
    compatibility and flexibility in configuration management.

    src/tx-manager.ts [45]

    -mpcContractId: string;
    +mpcContractId?: string;
     
    Suggestion importance[1-10]: 7

    Why: Making mpcContractId optional can improve backward compatibility and flexibility in configuration management, although it may not be necessary if the new design always requires this parameter.

    7

    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.

    1 participant