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

bug: default value doesn't exists as mentioned in docs #773

Closed
lambda-0x opened this issue Sep 28, 2023 · 9 comments
Closed

bug: default value doesn't exists as mentioned in docs #773

lambda-0x opened this issue Sep 28, 2023 · 9 comments
Labels
ODHack Issue to assign for the ODHack event OnlyDust Open for OnlyDust contributors Type: bug Something isn't working

Comments

@lambda-0x
Copy link

lambda-0x commented Sep 28, 2023

Describe the bug

I know this function is deprecated but i needed to use it for a CTF.

description of Invocation type says that the default value for signature is [].

but when its not specificed an error is thrown

    throw Error("formatSignature: provided signature is undefined");

To Reproduce
call invokeFunction without specifying signature field

Expected behavior
signature should use empty array as default value

Additional context
I am willing to open a PR for this if needed.

@lambda-0x lambda-0x added the Type: bug Something isn't working label Sep 28, 2023
@tabaktoni
Copy link
Collaborator

tabaktoni commented Oct 4, 2023

description of Invocation type says that the default value for signature is [].

I am not sure it says anything about the default value. It says it is undefined or ..., on that point, invokeFunction because of formatSignature does need sig even doe it is optional, this could be the issue referring to?
On the same track, I am not sure we should provide [] as the default when undefined signature.
Error is there to prevent unintentional undefined signature, but it should allow you to provide an empty array as a signature if you intentionally want to do it.

@penovicp @PhilippeR26

@PhilippeR26
Copy link
Collaborator

Note that Contract interface is :

public abstract populate(method: string, args?: ArgsOrCalldata): Invocation;

and contract/default.ts is :

public populate(method: string, args: RawArgs = []): Call {

Return type is not consistent between interface and implementation. Should be corrected (args type also).
Once corrected, this type is used only in Account.execute() and Contract.invoke() ; both functions will pass a mandatory Starknet standard signature to invokeFunction().
So, it seems that signature could be mandatory in Invoke (with probably impacts in cascade in the code).

@lambda-0x
Copy link
Author

sorry i meant to link this in the issue : https://www.starknetjs.com/docs/API/classes/Provider#invokefunction

image

Error is there to prevent unintentional undefined signature, but it should allow you to provide an empty array as a signature if you intentionally want to do it.

yeah that make sense, i think then we just need to update doc saying that its required field.

@lambda-0x
Copy link
Author

lambda-0x commented Oct 4, 2023

Once corrected, this type is used only in Account.execute() and Contract.invoke() ; both functions will pass a mandatory Starknet standard signature to invokeFunction().

So, it seems that signature could be mandatory in Invoke (with probably impacts in cascade in the code).

@PhilippeR26 starknet doesn't mandate signature, its upto the account contract implementer to decide how to use passed in signature, signature can be avoided altogether (althought that would be very rare)

without invokeFunction it wouldn't be possible to call an account contract whose keys you don't have. even thought they don't require it. is there any alternate way to do this (what i want basically is Account without private key)?

@PhilippeR26
Copy link
Collaborator

PhilippeR26 commented Oct 5, 2023

Once corrected, this type is used only in Account.execute() and Contract.invoke() ; both functions will pass a mandatory Starknet standard signature to invokeFunction().
So, it seems that signature could be mandatory in Invoke (with probably impacts in cascade in the code).

@PhilippeR26 starknet doesn't mandate signature, its upto the account contract implementer to decide how to use passed in signature, signature can be avoided altogether (althought that would be very rare)

without invokeFunction it wouldn't be possible to call an account contract whose keys you don't have. even thought they don't require it. is there any alternate way to do this (what i want basically is Account without private key)?

That's why i used the word standard about signature handled by Starknet.js. Today Starknet.js latest version v5.21.0 isn't coded to handle account abstraction about hash and signature. Even if you can use a very low level function to enter with some abstraction, nothing is made for handle all abstraction possibilities at high level.
It will come soon with merge of PR #748 .

@PhilippeR26
Copy link
Collaborator

And I have some difficulties to see the interest to pay to use a blockchain, using an account that do not provides any safety of a crypto signature.

@lambda-0x
Copy link
Author

lambda-0x commented Oct 5, 2023

And I have some difficulties to see the interest to pay to use a blockchain, using an account that do not provides any safety of a crypto signature.

there probably isn't a good reason for it directly. I just happened to need it in a CTF.

@tabaktoni tabaktoni added the OnlyDust Open for OnlyDust contributors label Oct 27, 2023
@ivpavici ivpavici added the ODHack Issue to assign for the ODHack event label Apr 19, 2024
@muheebyusufbaba1
Copy link

I will like to fix this for ODHack @ivpavici

@PhilippeR26
Copy link
Collaborator

In fact, this one should probably not be tagged as ODHack, and should have been closed.
To authorize a transaction without signature is a clear safety issue and should never occur.
I urge you to choose a different issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ODHack Issue to assign for the ODHack event OnlyDust Open for OnlyDust contributors Type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants