stellar / js-stellar-sdk

Main Stellar client library for the JavaScript language.
https://stellar.github.io/js-stellar-sdk/
Apache License 2.0
629 stars 313 forks source link

AssembledTransaction's `sign` should fail if no public key was provided #1059

Open Ifropc opened 4 weeks ago

Ifropc commented 4 weeks ago

We should fail fast if developer forgot to provide a public key (and default hardcoded NULL_ACCOUNT is used as the source account). This is a minor issue, but offers a better UX offering better error message "have you forgot to update publicKey?" instead of generic one

ShantelPeters commented 3 weeks ago

would love to work on this issue by tomorrow @Ifropc

onlydustapp[bot] commented 3 weeks ago

Hi @ShantelPeters! Maintainers during the ODHack # 8.0 will be tracking applications via OnlyDust. Therefore, in order for you to have a chance at being assigned to this issue, please apply directly here, or else your application may not be considered.

PoulavBhowmick03 commented 3 weeks ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I'm Poulav Bhowmick, a software engineer at Invisible Studios with a robust background in TypeScript, Rust, Solidity Cairo, fullstack development and blockchain technology. My experience includes building robust applications, optimizing functionalities and blockchain integration. I have actively participated in events and open source contributions, enhancing my capability to tackle real-world tech challenges. My projects can be viewed on my GitHub Profile and OnlyDust Profile. Plus I´m active member of Starknet, Ethereum ecosystem.

How I plan on tackling this issue

I will address this issue by implementing the following changes:

  1. Modify the sign method in the AssembledTransaction class:

    • Add a check at the beginning of the method to verify if a valid public key was provided
    • If the public key is missing or matches the NULL_ACCOUNT, throw a specific error
  2. Create a custom error class, e.g., MissingPublicKeyError, with a descriptive message: "No valid public key provided. Did you forget to update the publicKey?"

  3. Update the error handling in the sign method:

    • Catch the MissingPublicKeyError and any other relevant errors
    • Rethrow the error or wrap it in a more general error if needed
  4. Add unit tests to verify:

    • The sign method fails with the correct error when no public key is provided
    • The sign method fails with the correct error when the NULL_ACCOUNT is used
    • The sign method works correctly when a valid public key is provided
  5. Update documentation:

    • Add a note in the method's JSDoc about the new error condition
    • Update any relevant developer guides or README files to mention this change
  6. Consider adding a debug log or console warning in development mode to help catch this issue earlier in the development process

I'll submit a PR with these changes once implemented for review. This solution will provide a clearer error message to developers, helping them identify and fix the issue more quickly.

ETA - 2 days

raizo07 commented 3 weeks ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hello I'll like to be assigned to work on this.

I'm a software Dev with over four years experience and have worked on a couple of projects here. Here's a link to my profile https://app.onlydust.com/u/raizo07

SoarinSkySagar commented 3 weeks ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

GM, I am Sagar Rana, a smart contract developer and full stack engineer. I have 3 years of experience building robust full stack applications and over a year of writing smart contracts. You can see my projects and contributions to some major repos on my GitHub profile. The tech stack I use mainly includes Solidity, Rust, JavaScript and Typescript. I am also contributing to the Starknet and Rust ecosystems and building on Cairo and Rust languages. I am interested in contributing to projects like this to learn more about these technologies and help make these projects better. Please assign me as I would be really glad to be a contributor in this project! :)

How I plan on tackling this issue

Hi @lfropc, I would resolve this issue with the following approach:

Tasks:

ETA: 1 Day

MullerTheScientist commented 3 weeks ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I am a full-stack developer with experience in QA testing and languages like Python, Cairo, Solidity, React, and JavaScript.

How I plan on tackling this issue

i will Review Existing Code Examine the sign method implementation in AssembledTransaction. Identify the public key parameter or property. Add Public Key Validation Introduce a check at the beginning of the sign method Verify if the public key is present (not null, undefined, or empty). Throw an error if the public key is missing.

od-hunter commented 3 weeks ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

Hi, please can I be assigned this issue? I am a blockchain developer and I have experience in html, css, react, JavaScript,TypeScript and solidity, python and Cairo. I'd love to contribute to this repo please.

How I plan on tackling this issue

To solve this issue, I intend to take the following steps: 1.⁠ ⁠First of all, I’ll check for public key. If the public key is missing and the default hardcoded NULL_ACCOUNT is being used as the source account, you will trigger an error. 2.⁠ ⁠⁠I’ll then provide a Specific Error Message(instead of allowing a generic error to propagate, throw a specific error message) 3.⁠ ⁠⁠Next, I’ll locate the parts of the code where the public key is utilized and add the necessary validation to ensure the check is performed before any critical operations are executed. 4.⁠ ⁠⁠Lastly, I’ll write tests to confirm that the error handling works as expected when the public key is not provided.

Please assign me, I am ready to work.