Support V3 transactions in the Contract class

[ddoktorski]

Related to #1235
Initial PR #1255

Introduced changes

• PreparedFunctionCall class:
  • PreparedFunctionCall has only methods call() and call_raw()
  • Add abstract class PreparedFunctionInvoke and derive from it PreparedFunctionInvokeV1 and PreparedFunctionInvokeV3 with methods invoke() and estimate_fee()
• Contract class:
  • Break down declare() into:
    • declare_v1(), declare_v2() and declare_v3()
  • Break down deploy_contract() into:
    • deploy_contract_v1(), deploy_contract_v3() and add salt and unique parameters
• ContractFunction class:
  • Break down prepare() into:
    • prepare_invoke_v1(), prepare_invoke_v3() and prepare_call()
  • Break down invoke() into:
    • invoke_v1() and invoke_v3()
• DeclareResult class:
  • Break down deploy() into:
    • deploy_v1() and deploy_v3()
• Refactoring:
  • Move SentTransaction class to sent_transaction.py
  • Move ContractFunction, ContractData, InvokeResult and prepared call/invoke related classes to contract_function.py
  • Create contract_utils.py file
  • Move all Contract related classes to the starknet_py/net/contract directory

Please note that documentation update and support for Account.deploy_account() will be addressed in separate PRs.

• This PR contains breaking changes

[ddoktorski]

Regarding the refactor, splitting the code into files may not look perfect. It's tough to optimally split these closely tied classes without stumbling into circular import issues. If this refactoring doesn't make sense or complicates the review, let me know so I will revert these changes.

[codecov]

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (df9a651) 98.00% compared to head (1c52a28) 89.43%.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #1262      +/-   ##
===============================================
- Coverage        98.00%   89.43%   -8.58%     
===============================================
  Files               89       90       +1     
  Lines             4522     4582      +60     
===============================================
- Hits              4432     4098     -334     
- Misses              90      484     +394     

Files	Coverage Δ
starknet_py/contract_utils.py	83.33% <83.33%> (ø)
starknet_py/contract.py	87.90% <81.73%> (-11.72%)	⬇️

... and 54 files with indirect coverage changes

[THenry14]

I'm aware of the fact that we agreed to use postfixes. I do wonder, however, if in this particular case, where (prepare)_invoke(v1 | v3) can be differed by max_fee/resource_bounds and this is a helper class, would it make sense to have one prepare_invoke and one invoke, that takes those values as Union and based on type it prepares/makes v1 or v3 tx? wdyt? I know it may look like breaking existing approach to interface, but I do wonder if it does make sense in a helper class

[ddoktorski]

I definitely don't like the current interface. However, setting a fee as Union may not be the best approach:

• A unified name for max_fee and l1_resource_bounds would be required, let's say just fee. While this could make sense for a user with plenty of context, it might be confusing in general
• Thinking long term we will need to incorporate other V3 fields such as l2_resource_bounds, tip etc. As a result, we would need to alter this interface again

One potential solution is to create InvokeV1Params and InvokeV3Params and then use

prepare_invoke(params: InvokeParams)

However, this approach will break consistency with other parts of our codebase.

[THenry14]

Sure, it was just a rough idea either way. We can leave this as is.

[THenry14]

lgtm

[DelevoXDG]

Move all Contract related classes to the starknet_py/net/contract directory

Why this change?

[ddoktorski]

This is to improve file organization. I would prefer to move it to starknet_py/contract, but considering that we already have starknet_py/net/account in place, I think it makes sense to keep contract directory in starknet_py/net as well.

[ddoktorski]

After all, I decided to rollback the refactoring changes. This decision was driven by the user perspective, where it only created needless confusion. For instance, DeclareResult was found in the contract module, while InvokeResult resided in the contract_function module. I would prefer not to have everything in a single file, but I couldn't find a satisfactory way to logically divide these classes. Other than that, the only changes were made to address @DelevoXDG comments and fix docs build.

[DelevoXDG]

1. Do we really want to expose account there? It like a bit confusing property to have in PreparedFunctionInvokeVX, since it's only used as an internal helper and user would probably be aware which account they used to create prepared invoke anyway.
2. Also, could we maybe make _account non-optional? We can check whether it's None in prepare_invoke_vX before creating instance PreparedFunctionInvokeVX.

[ddoktorski]

1. The main motivation here is to avoid pyright errors when using _account. We are sure that it isn't None, as we make this verification in __post_init__ (excluding cases where a user might change it manually on an existing PreparedFunctionInvokeVX instance). Considering that an account is a prerequisite for transaction invocation, it kind of makes sense to have such a property in PreparedFunctionInvoke. As an alternative, I could make _get_account internal.
2. The account in the Contract class might be None, resulting in a None when calling prepare_invoke_vX. We can potentially make this assertion there, but then we would need to add this in four places instead of just one.

[DelevoXDG]

As an alternative, I could make _get_account internal.

That is one solution, because it's overly clear what it does in the context of PreparedFunctionInvokeVX

We can potentially make this assertion there, but then we would need to add this in four places instead of just one.

More like 4 instead of 2. Also, can we maybe check _account right away when it's passed to constructor?

[ddoktorski]

More like 4 instead of 2

It is implemented now only within the abstract PreparedFunctionInvoke class

can we maybe check _account right away when it's passed to constructor?

This is a dataclass, so the only option is __post_init__

[DelevoXDG]

lgtm 🔥