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

Add Account.deploy_account_v3 static method #1265

Merged

Conversation

ddoktorski
Copy link
Collaborator

Related to #1235 and #1262

Introduced changes

  • Add static method Account.deploy_account_v3()
  • Rename:
    • deploy_account -> deploy_account_v1
    • execute -> execute_v1
    • helper methods for parsing calls, previous names with v1 and v2 postfixes were confusing

Please note that documentation update will be done in a separate PR.

  • This PR contains breaking changes

StarknetChainId.GOERLI,
StarknetChainId.MAINNET,
):
if chain in StarknetChainId:
balance = await account.get_balance()
if balance < deploy_account_tx.max_fee:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure how to implement a similar check for the V3 transaction now. I'm guessing eventually we'd need to pull the balance from the STRK contract and an exchange rate to WEI?

Let me know if I am missing something here. Generally I think it is not crucial, but definitely nice to have.

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I suppose it could depend on fee DA mode as well, but I'm not 100% sure. Let's open an issue to investigate this, we might need to ask SW as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not crucial since tx will be rejected with a descriptive enough error message anyway

I'm guessing eventually we'd need to pull the balance from the STRK contract and an exchange rate to WEI?

It should probably be done the same way, except we'll have to pass STRK token address to get_balance. I don't really understand why'd you want to convert to WEI though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're absolutely right, there is no need for conversion. I thought that L1_GAS in the resource bounds is specified in WEI, even though V3 transaction costs are paid in FRI, which is not the case. Apologies for the confusion.

We can indeed check it the same way as for V1, but with the STRK contract. On the other hand, as @DelevoXDG said, it is not crucial, since it will throw an error anyway. @THenry14 Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, let's update #1266 with the findings and do necessary changes under that issue at some later date

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4d6175c) 98.00% compared to head (d14e1fa) 98.00%.

Additional details and impacted files
@@             Coverage Diff              @@
##           development    #1265   +/-   ##
============================================
  Coverage        98.00%   98.00%           
============================================
  Files               89       89           
  Lines             4513     4522    +9     
============================================
+ Hits              4423     4432    +9     
  Misses              90       90           
Files Coverage Δ
starknet_py/contract.py 99.62% <100.00%> (ø)
starknet_py/net/account/account.py 98.39% <100.00%> (+0.06%) ⬆️
starknet_py/net/account/base_account.py 100.00% <100.00%> (ø)

starknet_py/contract.py Show resolved Hide resolved
StarknetChainId.GOERLI,
StarknetChainId.MAINNET,
):
if chain in StarknetChainId:
balance = await account.get_balance()
if balance < deploy_account_tx.max_fee:
Copy link
Member

Choose a reason for hiding this comment

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

Good question. I suppose it could depend on fee DA mode as well, but I'm not 100% sure. Let's open an issue to investigate this, we might need to ask SW as well.

starknet_py/tests/e2e/account/account_test.py Show resolved Hide resolved
Copy link
Contributor

@DelevoXDG DelevoXDG left a comment

Choose a reason for hiding this comment

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

LGTM

starknet_py/net/account/account.py Outdated Show resolved Hide resolved
StarknetChainId.GOERLI,
StarknetChainId.MAINNET,
):
if chain in StarknetChainId:
balance = await account.get_balance()
if balance < deploy_account_tx.max_fee:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not crucial since tx will be rejected with a descriptive enough error message anyway

I'm guessing eventually we'd need to pull the balance from the STRK contract and an exchange rate to WEI?

It should probably be done the same way, except we'll have to pass STRK token address to get_balance. I don't really understand why'd you want to convert to WEI though?

@ddoktorski ddoktorski merged commit df9a651 into development Jan 31, 2024
14 checks passed
@ddoktorski ddoktorski deleted the ddoktorski/1235-rpc-0.6.0-Account-deploy-account branch January 31, 2024 11:50
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.

None yet

3 participants