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

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

Open
Ifropc opened this issue Sep 23, 2024 · 7 comments
Open

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

Ifropc opened this issue Sep 23, 2024 · 7 comments
Assignees
Labels

Comments

@Ifropc
Copy link

Ifropc commented Sep 23, 2024

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
Copy link

would love to work on this issue by tomorrow @Ifropc

Copy link

onlydustapp bot commented Sep 25, 2024

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
Copy link

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
Copy link

raizo07 commented Sep 26, 2024

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
Copy link

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:

  • Add a check in the sign method to see if a valid public key was provided (if its either not provided or a NULL_ADDRESS
  • Create a new error class with the provided message

Tasks:

  • Check for invalid public key in sign method
  • Create a new error
  • Implement the sign method with the custom error

ETA: 1 Day

@MullerTheScientist
Copy link

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Todo (Ready for Dev)
Development

Successfully merging a pull request may close this issue.

8 participants