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

[V7 Breaking] Fix/1252 #1255

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

[V7 Breaking] Fix/1252 #1255

wants to merge 8 commits into from

Conversation

tabaktoni
Copy link
Collaborator

Motivation and Resolution

Proposed solution for the Contract class method options
Chainable withOptions()
Fix #1252

RPC version (if applicable)

Usage related changes

Old

const status = await cairo1Contract.get_bet(1, {
          formatResponse: { name: 'string', description: 'string' },
        });

NEW

const status = await cairo1Contract
        .withOptions({
          formatResponse: { name: 'string', description: 'string' },
        })
        .get_bet(1);

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

@penovicp
Copy link
Collaborator

penovicp commented Nov 4, 2024

Copying what I wrote for PR#1253 about using spread Calldata as the argument so we can keep it in mind for this PR:

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.

The point made about the spread being filtered out later in the code execution still applies so the only supported scenario, updated for the changes from this PR, is the following:

  contract
    .withOptions({ parseRequest: false })
    .method(...Calldata)

An option is to keep it as is, but in that case we should probably clarify the behaviour in the documentation.

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.

splitArgsAndOptions with single argument containing options
2 participants