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

MPC Contract Upgrade (one yoco to dynamic) #98

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Aug 20, 2024

User description

MPC contract recently introduce a dynamic deposit based on signature request congestion.


PR Type

enhancement, dependencies


Description

  • Introduced a new constant MPC_MAX_DEPOSIT in src/chains/near.ts.
  • Updated src/mpcContract.ts to use MPC_MAX_DEPOSIT for deposit amounts instead of ONE_YOCTO.
  • Removed the unused ONE_YOCTO constant from src/chains/near.ts.

Changes walkthrough 📝

Relevant files
Enhancement
near.ts
Introduce `MPC_MAX_DEPOSIT` constant and remove `ONE_YOCTO`

src/chains/near.ts

  • Introduced MPC_MAX_DEPOSIT constant.
  • Removed unused ONE_YOCTO constant.
  • +2/-1     
    mpcContract.ts
    Update deposit amount to use `MPC_MAX_DEPOSIT`                     

    src/mpcContract.ts

  • Replaced ONE_YOCTO with MPC_MAX_DEPOSIT for deposit amounts.
  • Updated import to use MPC_MAX_DEPOSIT.
  • +3/-3     

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

    @mintbase-codium-pr-agent
    Copy link

    PR Reviewer Guide 🔍

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

    Comment Clarity
    The comment on line 6 might be unclear or misleading. It states "Unused amount will be returned to user (apparently)." This could be improved for better clarity and certainty.

    @mintbase-codium-pr-agent
    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Convert the string literal to a numeric literal for MPC_MAX_DEPOSIT to enhance type safety

    Replace the string literal for MPC_MAX_DEPOSIT with a numeric literal to ensure type
    safety and avoid potential runtime errors when performing arithmetic operations or
    comparisons.

    src/chains/near.ts [7]

    -export const MPC_MAX_DEPOSIT = "50000000000000000000000";
    +export const MPC_MAX_DEPOSIT = 50000000000000000000000n;
     
    Suggestion importance[1-10]: 10

    Why: Converting the string literal to a numeric literal for MPC_MAX_DEPOSIT ensures type safety and prevents potential runtime errors during arithmetic operations or comparisons. This is a significant improvement.

    10
    Security
    Add validation for MPC_MAX_DEPOSIT usage in transaction amounts and deposits

    Ensure that the amount and deposit fields in transaction calls are validated or
    checked to prevent misuse or errors, especially given the high value set in
    MPC_MAX_DEPOSIT.

    src/mpcContract.ts [80-100]

    -amount: MPC_MAX_DEPOSIT,
    -deposit: MPC_MAX_DEPOSIT,
    +amount: validateDeposit(MPC_MAX_DEPOSIT),
    +deposit: validateDeposit(MPC_MAX_DEPOSIT),
     
    Suggestion importance[1-10]: 9

    Why: Adding validation for the MPC_MAX_DEPOSIT ensures that the high value is checked before being used in transactions, which enhances security and prevents misuse or errors.

    9
    Robustness
    Add error handling around the signature transformation to manage transaction failures

    Implement error handling for the transaction methods to manage cases where the
    transaction might fail due to issues with the deposit amount or network errors.

    src/mpcContract.ts [83]

    -return transformSignature(mpcSig);
    +try {
    +  return transformSignature(mpcSig);
    +} catch (error) {
    +  console.error("Failed to transform signature:", error);
    +  throw error;
    +}
     
    Suggestion importance[1-10]: 8

    Why: Implementing error handling for the signature transformation enhances the robustness of the code by managing potential transaction failures due to deposit amount issues or network errors.

    8
    Maintainability
    Rename MPC_MAX_DEPOSIT to MAX_DEPOSIT_FOR_MPC_CONTRACT for better clarity

    Consider using a more descriptive and specific variable name than MPC_MAX_DEPOSIT to
    clarify its purpose and usage context in the contract, especially if it's used in
    multiple places with significant financial implications.

    src/mpcContract.ts [8]

    -import { TGAS, MPC_MAX_DEPOSIT } from "./chains/near";
    +import { TGAS, MAX_DEPOSIT_FOR_MPC_CONTRACT as MPC_MAX_DEPOSIT } from "./chains/near";
     
    Suggestion importance[1-10]: 7

    Why: Renaming MPC_MAX_DEPOSIT to a more descriptive name improves code readability and maintainability, making the purpose and usage context clearer.

    7

    @bh2smith bh2smith changed the title Contract upgrade MPC Contract Upgrade (one yoco to dynamic) Aug 20, 2024
    @bh2smith bh2smith merged commit 04f84b3 into main Aug 20, 2024
    1 check passed
    @bh2smith bh2smith deleted the contract-upgrade branch August 20, 2024 14:05
    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