Skip to content

Refactor outside execution functions #1537

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

Open
franciszekjob opened this issue Dec 14, 2024 · 29 comments
Open

Refactor outside execution functions #1537

franciszekjob opened this issue Dec 14, 2024 · 29 comments
Assignees
Labels

Comments

@franciszekjob
Copy link
Collaborator

franciszekjob commented Dec 14, 2024

Feature Request

  1. outside_execution_to_typed_data function should be a method of OutsideExecution, e.g.
@dataclass
class OutsideExecution:
    ...
    def to_typed_data(...) -> TypedData:

This requires resolving cycling imports errors, discussed here -> #1530 (comment)

  1. OutsideExecution. to_abi_dict() should ideally use schema while creating dict.
@baitcode
Copy link
Contributor

@franciszekjob I'm happy to tackle that issue, if you need help, of course. But would be nice to make a call to discuss.

@franciszekjob
Copy link
Collaborator Author

@franciszekjob I'm happy to tackle that issue, if you need help, of course. But would be nice to make a call to discuss.

Sure, you can address this once #1530 is done 😄

@kengoon
Copy link

kengoon commented Dec 15, 2024

I’d like to work on this.

@franciszekjob
Copy link
Collaborator Author

@kengoon sorry but @baitcode will be already tackling this issue.

@franciszekjob franciszekjob changed the title Refactor outside_execution_to_typed_data Refactor outside execution functions Dec 16, 2024
@pheobeayo
Copy link

Can I jump on this task?

@nottherealalanturing
Copy link

Let me try this one!

@franciszekjob
Copy link
Collaborator Author

franciszekjob commented Dec 20, 2024

@baitcode would you like to work on this one as we discussed?

@kengoon
Copy link

kengoon commented Dec 20, 2024

@franciszekjob if his answer is negative, I'm still up for this.

@cryptogru812
Copy link

I’m interested in this one.

@jaykayudo
Copy link

Is this issue still available?

@kengoon
Copy link

kengoon commented Jan 2, 2025

@franciszekjob It's been 13 days with no response, can I take on this?

@baitcode
Copy link
Contributor

baitcode commented Jan 3, 2025

@franciszekjob I'd love to but currently working on other issues. Can it wait for another week?

@nottherealalanturing
Copy link

@franciszekjob I can take it

@franciszekjob
Copy link
Collaborator Author

@franciszekjob I'd love to but currently working on other issues. Can it wait for another week?

@baitcode Yes

@BlessingEmejulu
Copy link

Can I start working on this?

@franciszekjob
Copy link
Collaborator Author

@BlessingEmejulu sorry, but another contributor will address it.

@emarc99
Copy link

emarc99 commented Jan 7, 2025

May I try my hand at this?

@Nityam573
Copy link

Can I take this from here?

@uzochukwuV
Copy link

I’d love to work on this task.

@baitcode
Copy link
Contributor

baitcode commented Jan 9, 2025

@franciszekjob Ready to start on 13th of Jan.

@franciszekjob
Copy link
Collaborator Author

@baitcode Ok, cool. Would you be also able to handle #1546?

@baitcode
Copy link
Contributor

@franciszekjob Yes, sure, that is an easy one.

@ekumamatthew
Copy link

I’m interested in this one.

@baitcode
Copy link
Contributor

@franciszekjob could you assign me to that one, please?

@franciszekjob
Copy link
Collaborator Author

@baitcode done

@baitcode
Copy link
Contributor

@franciszekjob I did some research and found some problems.

I see that the main problem that produces loads of cyclic dependency issues is module: starknet_py.utils.typed_data
The main issue with this one - it's widely used and at the same time it imports a lot of deep living internals into itself.

I see 2 possible ways of approaching this

With minimal changes but increasing entropy.

Move OutsideExecution to separate module (need help with naming here) import typed_data there and implement discussed functionality. Will not produce any cyclic imports.

In several steps trying to improve module connectivity.

Lots of changes, probably will introduce some backward incompatible ones. I don't think I'll ever get approve for such a change. But at least I'll document my thoughts.
The idea is to make typed_data importable in starknet_py.net.client_models. I see several vectors of achieving that. Not sure all of those recommedations need to be implemented. But some of those definitely make sense and worth at least considering.

starknet_py/utils/typed_data.py simplification
The idea is to use Serialiser to replace the way _encode_data is preparing data for struct_hash().

split starknet_py.net.client_models

  • move enums into subpackage importing indirectly
  • mode schema dependent models

Import flow fix
starknet_py/utils internally use starknet_py/net/models
but judging from the structure, modules that are deeper in the tree should use parent utils and their own utils. but not the other way around, otherwise cyclic imports are hard to avoid.

I would propose to remove dependency:

  • on _to_rpc_felt by moving method into utils.py and making it public - this will require a huge update as it is used a lot in the project and is imported indirectly
  • from starknet_py.net.models.typed_data import DomainDict, Revision, TypedDataDict see point 2.
  • from starknet_py.serialization.data_serializers import ByteArraySerializer - it would be absolutely awesome if data for struct_hash was generated using Serialiser, I'm happy to try to implement this one.

Merge starknet_py/utils/typed_data.py and starknet_py/net/models/typed_data.py

those files contain similar data structures, I haven't yet fully understood what are the responsibilities
Possible solutions:

  • group all logic in starknet_py/utils/typed_data.py
  • remove from_dict method on the starknet_py/utils/typed_data.py classes and and create to_typed_dict on the net/models classes and rewrite usage.

@franciszekjob
Copy link
Collaborator Author

franciszekjob commented Feb 16, 2025

Hi @baitcode 👋 , thanks for the deep dive into the problem! I think we should choose option 2, because these cyclic imports are really problematic in the long term, so it's best if we get rid of them. Will it brings lots of breaking changes? Would you like to start working on this?

@baitcode
Copy link
Contributor

@franciszekjob Sure will do. But I wan't to approach it in several steps. Will outline those here a bit later.

@franciszekjob
Copy link
Collaborator Author

@baitcode sure, ping me whenever you need 👍

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

No branches or pull requests