Skip to content

Conversation

@tomek0123456789
Copy link
Contributor

@tomek0123456789 tomek0123456789 commented Sep 14, 2023

Closes #1182

Introduced changes

  • add cairo_version parameter to Account constructor and Account.deploy_account method
  • add cairo_version property

  • This PR contains breaking changes

@tomek0123456789 tomek0123456789 force-pushed the tomek/1182-cairo1-accounts branch from ec2e124 to 6907be0 Compare September 15, 2023 14:32
@tomek0123456789 tomek0123456789 marked this pull request as ready for review September 15, 2023 14:43
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.13% 🎉

Comparison is base (ff2639e) 97.44% compared to head (8a4ceed) 97.58%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #1183      +/-   ##
===============================================
+ Coverage        97.44%   97.58%   +0.13%     
===============================================
  Files               92       92              
  Lines             4661     4680      +19     
===============================================
+ Hits              4542     4567      +25     
+ Misses             119      113       -6     
Files Changed Coverage Δ
starknet_py/net/account/account.py 98.99% <100.00%> (+0.08%) ⬆️
starknet_py/net/account/base_account.py 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@drknzz drknzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍
Let's just have a second look around the library to make sure no place with account was omitted.

Copy link

@ClementWalter ClementWalter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you should get the cairo_version completely internal, and set it async based on the address in the fixture, though the current code works as well


@property
def cairo_version(self) -> int:
return self._cairo_version

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you could do instead something like

@property
async def cairo_version(self) -> int:
    if self._cairo_version is None:
        # Get the class hash of self.address
        class_ = await self._client.get_class_at(self.address)
        self.cairo_version = 1 if isinstance(class, Sierra) else 0
    return self._cairo_version

Copy link
Contributor Author

@tomek0123456789 tomek0123456789 Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if an async property would be a good UI. However, we've got a sync interface for that, so something like this would work without awaiting:

@property
def cairo_version(self) -> int:
    if self._cairo_version is None:
        if isinstance(self._client, GatewayClient):
            contract_class = self._client.get_full_contract_sync(contract_address=self._address)
        else:
            contract_class = self.client.get_class_at_sync(contract_address=self._address)
        self._cairo_version = 1 if isinstance(contract_class, SierraContractClass) else 0
    return self._cairo_version

wdyt?
edit: this breaks a lot of things, but the same question applies, is an async property a good UI?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, just see this comment now. Async property you're right may not have been the best thing, but with sync definitely as It's quite common to use property to cache a given computed/fetched data

there is no information regarding the internal implementation for the outside world, use you still use self.cairo_version

@tomek0123456789 tomek0123456789 merged commit 79b6659 into development Sep 19, 2023
@tomek0123456789 tomek0123456789 deleted the tomek/1182-cairo1-accounts branch September 19, 2023 14:32
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.

Add cairo_version parameter to PreparedFunctionCall.invoke

4 participants