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

fix: contract class splitArgsAndOptions #1253

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

fix: contract class splitArgsAndOptions #1253

wants to merge 3 commits into from

Conversation

tabaktoni
Copy link
Collaborator

@tabaktoni tabaktoni commented Oct 31, 2024

Motivation and Resolution

fix type splitArgsAndOptions

Usage related changes

type updated to represent better what it is

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

Copy link
Collaborator

@penovicp penovicp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the ArgsOrCalldataWithOptions type is accurate, the ContractOptions object is used as a concatenation to the regular parameters of the encapsulated method and these get passed to the splitArgsAndOptions utility. Example from the tests: link. So the type would be closer to something like this: [...ArgsOrCalldata, ContractOptions?]

The issue boils down to differentiating whether the last element is a ContractOptions or a RawArgsObject or MultiType object.

@tabaktoni
Copy link
Collaborator Author

tabaktoni commented Oct 31, 2024

I don't think the ArgsOrCalldataWithOptions type is accurate, the ContractOptions object is used as a concatenation to the regular parameters of the encapsulated method and these get passed to the splitArgsAndOptions utility. Example from the tests: link. So the type would be closer to something like this: [...ArgsOrCalldata, ContractOptions?]

The issue boils down to differentiating whether the last element is a ContractOptions or a RawArgsObject or MultiType object.

Yea it is [ArgsOrCalldataWithOptions] actually as args are array.
What you sad is that ArgsOrCalldata part's can also be an object, what I was thinking could be possible, as I think RawArgsArray is incorrect and should be like RawArgs and all wrap in (args)array.
I will need to investigate this, but if that is an case we have type issue and a problem to resolve this.

@tabaktoni
Copy link
Collaborator Author

OK i belave at least now i fixed type

@penovicp
Copy link
Collaborator

penovicp commented Nov 1, 2024

Yes, that is a more accurate representation.

I believe the spread Calldata branch can also be removed ([...Calldata] | [...Calldata, ContractOptions] from the reworked ArgsOrCalldataWithOptions type). It is supported by splitArgsAndOptions itself, however, splitArgsAndOptions is in all its usages followed by the getCalldata() utility that filters the argument based on the __compiled__ marker which isn't propagated if the Calldata is spread. This means that the only spread input that should be able to work is method(...Calldata, { parseRequest: false }).

I don't see any utility in restoring the full support, so refactoring the code to exclude the supported scenario seems preferable, it should also simplify detecting whether a ContractOptions object is used.

@tabaktoni
Copy link
Collaborator Author

I agree focused on this pr to fix type and new one to propose solution

@penovicp penovicp mentioned this pull request Nov 4, 2024
6 tasks
@@ -21,8 +21,37 @@ export type Result =
| boolean
| CairoEnum;

export type ArgsOrCalldata = RawArgsArray | [Calldata] | Calldata;
export type ArgsOrCalldataWithOptions = ArgsOrCalldata & ContractOptions;
// export type ArgsOrCalldata = RawArgsArray | [Calldata] | Calldata;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only q I have is if these are left intentionally here commented out?

Copy link
Collaborator

@PhilippeR26 PhilippeR26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No remarks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants