starknet.js icon indicating copy to clipboard operation
starknet.js copied to clipboard

fix: contract class splitArgsAndOptions

Open tabaktoni opened this issue 1 year ago • 4 comments

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

tabaktoni avatar Oct 31 '24 11:10 tabaktoni

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 avatar Oct 31 '24 16:10 tabaktoni

OK i belave at least now i fixed type

tabaktoni avatar Nov 01 '24 13:11 tabaktoni

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.

penovicp avatar Nov 01 '24 14:11 penovicp

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

tabaktoni avatar Nov 01 '24 15:11 tabaktoni