starknet.js
starknet.js copied to clipboard
fix: contract class splitArgsAndOptions
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
I don't think the
ArgsOrCalldataWithOptionstype is accurate, theContractOptionsobject is used as a concatenation to the regular parameters of the encapsulated method and these get passed to thesplitArgsAndOptionsutility. 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
ContractOptionsor aRawArgsObjectorMultiTypeobject.
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.
OK i belave at least now i fixed type
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.
I agree focused on this pr to fix type and new one to propose solution