Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

Auto calculate max_fee #332

Merged
merged 31 commits into from
Jan 9, 2023
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
3a01ca9
add estimate_fee_if_zero
andrew-fleming Jan 5, 2023
01e93ed
add tests for estimate_fee_if_zero
andrew-fleming Jan 5, 2023
56c825d
add estimate_fee to run_transaction
andrew-fleming Jan 5, 2023
9a9af6f
src/nile/nre.py
andrew-fleming Jan 5, 2023
1a91cce
formatting
andrew-fleming Jan 5, 2023
8af83d5
remove return
andrew-fleming Jan 5, 2023
b0465aa
Apply suggestions from code review
andrew-fleming Jan 6, 2023
01237cc
fix func name
andrew-fleming Jan 6, 2023
7aed394
remove nre tx method
andrew-fleming Jan 6, 2023
e74b5fe
add auto_fee param to execute
andrew-fleming Jan 6, 2023
e8591d2
Revert "add auto_fee param to execute"
andrew-fleming Jan 6, 2023
c22e552
change default max_fee to auto
andrew-fleming Jan 6, 2023
3606bde
add conditional for auto max_fee
andrew-fleming Jan 6, 2023
621acea
fix _process_args
andrew-fleming Jan 6, 2023
819c269
remove import
andrew-fleming Jan 6, 2023
de70392
remove estimate_fee func
andrew-fleming Jan 6, 2023
dfd2ee4
remove estimate fee test
andrew-fleming Jan 6, 2023
c33ac99
adjust tests for auto max_fee
andrew-fleming Jan 6, 2023
26ebcba
Revert "add conditional for auto max_fee"
andrew-fleming Jan 6, 2023
f29487c
remove unused param
andrew-fleming Jan 6, 2023
96cc558
add set_estimate_fee_if_auto
andrew-fleming Jan 6, 2023
d5035ec
roll back auto fee changes
andrew-fleming Jan 7, 2023
5c0451b
add estimate_fee func to methods
andrew-fleming Jan 7, 2023
8eca7c5
normalize max_fee if None
andrew-fleming Jan 7, 2023
e4bbc57
adjust tests for max_fee if None
andrew-fleming Jan 7, 2023
c79ea15
fix test
andrew-fleming Jan 7, 2023
0f0918e
Apply suggestions from code review
andrew-fleming Jan 8, 2023
4dd51a1
fix max_fee handling
andrew-fleming Jan 8, 2023
3d115a4
fix tests
andrew-fleming Jan 8, 2023
5a320a4
remove comments
andrew-fleming Jan 8, 2023
b4dbb9e
Update src/nile/core/types/account.py
andrew-fleming Jan 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions src/nile/core/types/account.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,17 +101,20 @@ async def deploy(self, salt=None, max_fee=None, abi=None):
predicted_address=predicted_address,
overriding_path=NILE_ARTIFACTS_PATH,
calldata=calldata,
max_fee=max_fee,
max_fee=max_fee or 0,
network=self.network,
)

return DeployAccountTxWrapper(
tx_wrapper = DeployAccountTxWrapper(
tx=transaction,
account=self,
alias=self.alias,
abi=abi,
)

await _set_estimated_fee_if_none(max_fee, tx_wrapper)
return tx_wrapper

async def send(
self,
address_or_alias,
Expand All @@ -133,16 +136,19 @@ async def send(
transaction = InvokeTransaction(
account_address=self.address,
calldata=execute_calldata,
max_fee=max_fee,
max_fee=max_fee or 0,
nonce=nonce,
network=self.network,
)

return InvokeTxWrapper(
tx_wrapper = InvokeTxWrapper(
tx=transaction,
account=self,
)

await _set_estimated_fee_if_none(max_fee, tx_wrapper)
return tx_wrapper

async def declare(
self,
contract_name,
Expand All @@ -163,18 +169,21 @@ async def declare(
transaction = DeclareTransaction(
account_address=self.address,
contract_to_submit=contract_name,
max_fee=max_fee,
max_fee=max_fee or 0,
nonce=nonce,
network=self.network,
overriding_path=overriding_path,
)

return DeclareTxWrapper(
tx_wrapper = DeclareTxWrapper(
tx=transaction,
account=self,
alias=alias,
)

await _set_estimated_fee_if_none(max_fee, tx_wrapper)
return tx_wrapper

async def deploy_contract(
self,
contract_name,
Expand Down Expand Up @@ -204,12 +213,12 @@ async def deploy_contract(
unique=unique,
calldata=calldata,
deployer_address=deployer_address,
max_fee=max_fee,
max_fee=max_fee or 0,
nonce=nonce,
overriding_path=overriding_path,
)

return DeployContractTxWrapper(
tx_wrapper = DeployContractTxWrapper(
tx=transaction,
account=self,
alias=alias,
Expand All @@ -219,6 +228,9 @@ async def deploy_contract(
abi=abi,
)

await _set_estimated_fee_if_none(max_fee, tx_wrapper)
return tx_wrapper

def _get_target_address(self, address_or_alias):
if not is_alias(address_or_alias):
target_address = normalize_number(address_or_alias)
Expand All @@ -233,7 +245,8 @@ def _get_target_address(self, address_or_alias):
return target_address

async def _process_arguments(self, max_fee, nonce, calldata=None):
max_fee = 0 if max_fee is None else int(max_fee)
if max_fee is not None:
max_fee = int(max_fee)

if nonce is None:
nonce = await get_nonce(self.address, self.network)
Expand All @@ -255,6 +268,13 @@ def _get_signer_and_alias(signer, predeployed_info):
return signer, alias


async def _set_estimated_fee_if_none(max_fee, tx):
"""Estimate max_fee for transaction if max_fee is None."""
if max_fee is None:
estimated_fee = await tx.estimate_fee()
tx.update_fee(estimated_fee)
Copy link
Member

@ericnordelo ericnordelo Jan 7, 2023

Choose a reason for hiding this comment

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

I think with this we are good to go, just a small suggestion for organization.

Keeping in mind we are calling this for all the account methods returning tx wrappers. Can't we add this logic in the BaseTxWrapper __post_init__ method to avoid having to call the _set_estimated_fee_if_none method repeatedly in account?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the idea early on, but we'd need __post_init__ to be async in order to call the underlying tx.estimate_fee. It should be possible to do, but I think it's a bit hacky (a la the Account initialization with the async self.deploy)

Copy link
Member

Choose a reason for hiding this comment

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

Got it, makes sense.

andrew-fleming marked this conversation as resolved.
Show resolved Hide resolved


async def try_get_account(
signer,
network,
Expand Down
2 changes: 1 addition & 1 deletion src/nile/core/types/transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def __post_init__(self):
# Validate the transaction object
self._validate()

async def execute(self, signer, max_fee=None, watch_mode=None, **kwargs):
async def execute(self, signer, watch_mode=None, **kwargs):
"""Execute the transaction."""
sig_r, sig_s = signer.sign(message_hash=self.hash)

Expand Down
12 changes: 10 additions & 2 deletions tests/core/types/test_account.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,9 @@ async def test_deploy(
@patch("nile.core.types.account.get_counterfactual_address", return_value=MOCK_ADDRESS)
@patch("nile.core.types.transactions.get_class_hash", return_value=CLASS_HASH)
@patch("nile.core.deploy.accounts.register")
@patch("nile.core.types.account._set_estimated_fee_if_none")
async def test_deploy_accounts_register(
mock_register, mock_hash, mock_address, mock_deploy
mock_set_fee, mock_register, mock_hash, mock_address, mock_deploy
):
account = await Account(KEY, NETWORK)

Expand Down Expand Up @@ -187,9 +188,16 @@ async def test_declare(
@patch("nile.core.types.transactions.get_contract_class", return_value="ContractClass")
@patch("nile.core.types.account.DeclareTransaction", return_value="tx")
@patch("nile.core.types.account.DeclareTxWrapper")
@patch("nile.core.types.account._set_estimated_fee_if_none")
async def test_declare_account(
mock_tx_wrapper, mock_tx, mock_contract_class, nile_account, overriding_path
mock_set_fee,
mock_tx_wrapper,
mock_tx,
mock_contract_class,
nile_account,
overriding_path,
):
# mock_tx_wrapper = AsyncMock()
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be here?

Copy link
Contributor Author

@andrew-fleming andrew-fleming Jan 25, 2023

Choose a reason for hiding this comment

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

It should not!

#345

account = await MockAccount(KEY, NETWORK)

contract_name = "Account"
Expand Down
6 changes: 3 additions & 3 deletions tests/core/types/test_transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ async def test_invoke_transaction_init(
assert tx.entry_point == entry_point
assert tx.calldata == calldata
assert tx.account_address == account_address
assert tx.max_fee == max_fee
assert tx.max_fee == max_fee or 0
assert tx.nonce == nonce
assert tx.network == network
assert tx.version == version
Expand Down Expand Up @@ -118,7 +118,7 @@ async def test_declare_transaction_init(
assert tx.contract_class == "ContractClass"
assert tx.overriding_path == overriding_path
assert tx.account_address == account_address
assert tx.max_fee == max_fee
assert tx.max_fee == max_fee or 0
assert tx.nonce == nonce
assert tx.network == network
assert tx.version == version
Expand Down Expand Up @@ -175,7 +175,7 @@ async def test_deploy_account_transaction_init(
assert tx.class_hash == 777
assert tx.overriding_path == overriding_path
assert tx.account_address == account_address
assert tx.max_fee == max_fee
assert tx.max_fee == max_fee or 0
assert tx.nonce == nonce
assert tx.network == network
assert tx.version == version
Expand Down
2 changes: 1 addition & 1 deletion tests/core/types/test_udc_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ async def test_create_udc_deploy_transaction(
unique,
calldata,
deployer_address,
max_fee,
max_fee or 0,
nonce=nonce,
)

Expand Down