Skip to content

[FIX] - Fixed Ui Wallet Handling#50

Merged
MemoAlfa merged 1 commit intomainfrom
myym/ui-wallet-handling
Apr 1, 2025
Merged

[FIX] - Fixed Ui Wallet Handling#50
MemoAlfa merged 1 commit intomainfrom
myym/ui-wallet-handling

Conversation

@YameenMalik
Copy link
Copy Markdown
Contributor

@YameenMalik YameenMalik commented Mar 31, 2025

User description

The Bluefin Signer class does not cater to UI wallet, updated it.


PR Type

Enhancement, Bug fix


Description

  • Updated BluefinRequestSigner to support UI wallets.

  • Replaced makeAddressableKeyPair with makeSigner for wallet handling.

  • Fixed naming inconsistencies in deauthorize account request functions.

  • Added isUIWallet method to determine wallet type.


Changes walkthrough 📝

Relevant files
Enhancement
example.ts
Update example to use `makeSigner` for wallet handling     

ts/sdk/example.ts

  • Replaced makeAddressableKeyPair with makeSigner for wallet handling.
  • Updated BluefinRequestSigner instantiation to include UI wallet
    support.
  • +2/-2     
    request-signer.ts
    Enhance `BluefinRequestSigner` and wallet handling logic 

    ts/sdk/src/request-signer.ts

  • Replaced IAddressable with ISigner interface for wallet handling.
  • Added isUIWallet method to ISigner and BluefinRequestSigner.
  • Renamed UIDeauthorizeAccountRequest to UIDeAuthorizeAccountRequest.
  • Updated executeTx to handle UI wallet type.
  • +25/-14 
    Formatting
    sdk.ts
    Minor formatting updates in SDK options                                   

    ts/sdk/src/sdk.ts

    • Minor formatting changes in BluefinProSdkOptions.
    +0/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @YameenMalik YameenMalik requested a review from a team March 31, 2025 21:10
    @qodo-code-review
    Copy link
    Copy Markdown

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Incomplete Implementation

    The executeTx method now passes an additional parameter (isUIWallet) to SuiBlocks.execCall, but we don't see the implementation of SuiBlocks.execCall to verify it properly handles this new parameter.

    return SuiBlocks.execCall(txb, suiClient, this.wallet, false, this.wallet.isUIWallet());
    Type Consistency

    The wallet parameter type has changed from IAddressable to ISigner, but the Pick<Signer, "signPersonalMessage"> part remains. Verify that this combination works correctly with UI wallets.

    private readonly wallet: Pick<Signer, "signPersonalMessage"> & ISigner

    @qodo-code-review
    Copy link
    Copy Markdown

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add deprecation notice

    The getAddress() method in BluefinRequestSigner now calls toSuiAddress() instead
    of the old getAddress(), but this could cause confusion since the method names
    don't match. Consider renaming the method to toSuiAddress() for consistency or
    adding a deprecation notice.

    ts/sdk/src/request-signer.ts [339-341]

    +/**
    + * Get the wallet's address
    + * @deprecated Consider using toSuiAddress() for consistency with the ISigner interface
    + */
     getAddress(): string {
       return this.wallet.toSuiAddress();
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion adds a deprecation notice to the getAddress() method, which is important for API consistency since the underlying implementation now uses toSuiAddress(). This helps prevent confusion and guides developers toward using the preferred method.

    Medium
    Add clarifying comment

    The makeSigner function is called with only two arguments, but the second
    parameter isUIWallet is not clearly documented in the example. Consider adding a
    comment to explain what this boolean parameter represents for better code
    clarity.

    ts/sdk/example.ts [105]

    +// false indicates this is not a UI wallet (e.g., browser extension)
     const bfSigner = new BluefinRequestSigner(makeSigner(suiWallet, false));
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion adds a helpful comment that clarifies the purpose of the boolean parameter in the makeSigner function, which improves code readability and helps future developers understand the API usage.

    Low
    • More

    @MemoAlfa MemoAlfa merged commit b6b7c8d into main Apr 1, 2025
    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.

    3 participants