Auto calculate max_fee #332

andrew-fleming · 2023-01-05T09:08:32Z

Resolves #284.

This PR proposes to automatically estimate the max_fee when the max_fee isn't explicitly passed (which would be interpreted as 0). The fee estimation logic lies in estimate_fee_when_zero.

Some notes:

CLI

estimate_fee_when_zero precedes tx.execute in the run_transaction function.

NRE

The NRE includes a new method execute_tx which estimates the fee (if zero) and executes the transaction. If accepted, the proposed changes will then offer users two ways to script account txs:

    await nre.execute_tx(
        account.send(ETH_TOKEN_ADDRESS, "transfer", [recipient, *amount])
    )

    OR:

    tx = await account.send(ETH_TOKEN_ADDRESS, "transfer", [recipient, *amount])
    max_fee = await tx.estimate_fee()
    await tx.update_fee(max_fee).execute()

ericnordelo

Looking good Andrew! Glad to see no big changes were required from 320. Left some comments.

src/nile/common.py

ericnordelo · 2023-01-05T12:50:16Z

src/nile/nre.py

@@ -33,6 +33,12 @@ def call(self, address_or_alias, method, params=None, abi=None):
            address_or_alias, "call", method, params, self.network, abi=abi
        )

+    async def execute_tx(self, tx_wrapper, watch_mode=None):
+        """Add estimated max_fee (if max_fee is zero) and execute transaction."""
+        tx = await tx_wrapper


I think is better to keep the convention of receiving objects and not promises (for the sake of the API simplicity).

I understand that the idea is to allow:

await nre.execute_tx( account.send(ETH_TOKEN_ADDRESS, "transfer", [recipient, *amount]) )

instead of

await nre.execute_tx( await account.send(ETH_TOKEN_ADDRESS, "transfer", [recipient, *amount]) )

But I think it will be confusing for a user trying to achieve something like this:

tx = await account.send(ETH_TOKEN_ADDRESS, "transfer", [recipient, *amount]) await nre.execute_tx(tx)

And the last looks cleaner IMO. What do you think?

Yeah, that sounds good to me. I was trying just trying to simplify as much as possible, I agree that your proposed solution looks cleaner

ericnordelo · 2023-01-05T13:04:13Z

src/nile/nre.py

@@ -33,6 +33,12 @@ def call(self, address_or_alias, method, params=None, abi=None):
            address_or_alias, "call", method, params, self.network, abi=abi
        )

+    async def execute_tx(self, tx_wrapper, watch_mode=None):


Keeping in mind that this method is modifying the tx (not only executing it), I would rename it to something more descriptive. Not sure what name to use though, maybe set_estimated_fee_and_execute? This name doesn't take into account that if the fee is different than 0 the estimated one won't be used, so I think is still confusing.

Keeping in mind that execute_tx updates tx.hash, tx.max_fee, and tx.query_hash, users can get unexpected behavior on the script by calling this execute_tx method without knowing that it modifies the tx object.

I'm more in favor of keeping only this way, which is more explicit and easier to follow because of that:

tx = await account.send(ETH_TOKEN_ADDRESS, "transfer", [recipient, *amount]) max_fee = await tx.estimate_fee() await tx.update_fee(max_fee).execute()

Keeping in mind that this method is modifying the tx (not only executing it), I would rename it to something more descriptive

Agreed. The idea was to have a simple and easy-to-use method. It's probably better to have set_estimated_fee_and_execute if we keep this approach

users can get unexpected behavior on the script by calling this execute_tx method without knowing that it modifies the tx object

I'm more in favor of keeping only this way, which is more explicit and easier to follow because of that:

My only contention with this consists of the "auto calculate fee" issue itself because we would be asking the user to set up the update_fee and it wouldn't exactly be automated in the NRE (though, the flow makes it quite easy). I suppose the question is: do you think the tx.update_fee flow is enough to satisfy the issue?

Got you. I think having the current design, in NRE update_fee is enough to satisfy because issues by misleading tx hashes after execution weigh more than the benefit over the two extra lines IMO (in CLI happens transparently).

Maybe we could implement an "auto fee" mechanism in Transaction initialization that would initialize the transaction with the estimated fee, transparently to the user (if the max_fee in construction time is set to -1 or "auto" in initialization for example), what do you think?

I think that's a better idea! And it's implemented, I just need to update tests

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

This reverts commit e74b5fe.

ericnordelo · 2023-01-06T14:08:44Z

src/nile/core/types/transactions.py

        """Execute the transaction."""
+        if self.max_fee == 0:
+            max_fee = await self.estimate_fee(signer)
+            self.update_fee(max_fee)


The rationale for me not adding the max_fee directly to the execute method was the fact that the transaction was going to be updated (tx.hash, and tx.max_fee) without the user noting this.

The same thing happens if we add this logic here. When I commented about the "auto" option, I was thinking of something in construction time, not in execution. I still think changing the tx.hash in execute without the user noting is not the best design.

In construction time, at least the tx.hash and the max_fee will be set to the estimated fee from the beginning. So something like this won't be possible:

# Bad design tx = await account.send(max_fee=0...) print(f"Sending: {tx.hash}") # The printed hash is not the correct one, because this would change it await tx.execute()

I also think that we should let the user set the max_fee as 0, that's why I think "auto" would be a better option for auto fee estimation. Then we know the user won't receive a different transaction hash from the one he was expecting (it could be one with max_fee equal 0, even if it doesn't make much sense to us, it could make sense).

Ah forgive me, I lost sight of the tx.hash changing. The issue I was running into with that approach is estimate_fee is async, so I think executing this in during construction (with the sync __post_init__) won't work without getting into hacky territory. Because of the async issue, I don't think we can pass auto or -1 to the Transaction during construction either.

Two options that I see are:

Have the users use tx.update_fee which IMO isn't automating the fee but as you mentioned, maybe it's enough

Automate the fee in the account methods which keeps the tx.hash consistent for the user

Number 2 sounds great. Returning the tx_wrapper with the estimated fee if max_fee is left to None in account methods.

This reverts commit 3606bde.

ericnordelo · 2023-01-07T09:48:02Z

src/nile/core/types/account.py

+    if max_fee is None:
+        estimated_fee = await tx.estimate_fee()
+        tx.update_fee(estimated_fee)


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?

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)

Got it, makes sense.

src/nile/core/types/transactions.py

ericnordelo

Almost there! I left a couple of comments to keep the Transaction code as clean as possible, and I think we are good to go.

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

ericnordelo

Almost there :)!

src/nile/core/types/account.py

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

ericnordelo

LGTM!

martriay · 2023-01-25T07:04:58Z

tests/core/types/test_account.py

 ):
+    # mock_tx_wrapper = AsyncMock()


should this be here?

It should not!

#345

andrew-fleming added 6 commits January 5, 2023 03:20

add estimate_fee_if_zero

3a01ca9

add tests for estimate_fee_if_zero

01e93ed

add estimate_fee to run_transaction

56c825d

src/nile/nre.py

9a9af6f

formatting

1a91cce

remove return

8af83d5

andrew-fleming requested a review from ericnordelo January 5, 2023 09:08

ericnordelo reviewed Jan 5, 2023

View reviewed changes

andrew-fleming and others added 12 commits January 5, 2023 19:49

Apply suggestions from code review

b0465aa

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

fix func name

01237cc

remove nre tx method

7aed394

add auto_fee param to execute

e74b5fe

Revert "add auto_fee param to execute"

e8591d2

This reverts commit e74b5fe.

change default max_fee to auto

c22e552

add conditional for auto max_fee

3606bde

fix _process_args

621acea

remove import

819c269

remove estimate_fee func

de70392

remove estimate fee test

dfd2ee4

adjust tests for auto max_fee

c33ac99

ericnordelo reviewed Jan 6, 2023

View reviewed changes

andrew-fleming added 8 commits January 6, 2023 12:16

Revert "add conditional for auto max_fee"

26ebcba

This reverts commit 3606bde.

remove unused param

f29487c

add set_estimate_fee_if_auto

96cc558

roll back auto fee changes

d5035ec

add estimate_fee func to methods

5c0451b

normalize max_fee if None

8eca7c5

adjust tests for max_fee if None

e4bbc57

fix test

c79ea15

ericnordelo reviewed Jan 7, 2023

View reviewed changes

src/nile/core/types/transactions.py Outdated Show resolved Hide resolved

ericnordelo reviewed Jan 7, 2023

View reviewed changes

src/nile/core/types/transactions.py Outdated Show resolved Hide resolved

ericnordelo reviewed Jan 7, 2023

View reviewed changes

andrew-fleming and others added 4 commits January 7, 2023 19:33

Apply suggestions from code review

0f0918e

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

fix max_fee handling

4dd51a1

fix tests

3d115a4

remove comments

5a320a4

ericnordelo reviewed Jan 8, 2023

View reviewed changes

src/nile/core/types/account.py Outdated Show resolved Hide resolved

Update src/nile/core/types/account.py

b4dbb9e

Co-authored-by: Eric Nordelo <eric.nordelo39@gmail.com>

ericnordelo approved these changes Jan 9, 2023

View reviewed changes

ericnordelo merged commit 08fb7be into OpenZeppelin:main Jan 9, 2023

andrew-fleming deleted the add-auto-max_fee branch January 10, 2023 23:28

martriay reviewed Jan 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto calculate max_fee #332

Auto calculate max_fee #332

andrew-fleming commented Jan 5, 2023

ericnordelo left a comment

ericnordelo Jan 5, 2023

andrew-fleming Jan 5, 2023

ericnordelo Jan 5, 2023

andrew-fleming Jan 5, 2023

ericnordelo Jan 5, 2023 •

edited

Loading

andrew-fleming Jan 6, 2023

ericnordelo Jan 6, 2023

ericnordelo Jan 6, 2023 •

edited

Loading

andrew-fleming Jan 6, 2023

ericnordelo Jan 6, 2023

andrew-fleming Jan 6, 2023

ericnordelo Jan 7, 2023 •

edited

Loading

andrew-fleming Jan 7, 2023

ericnordelo Jan 7, 2023

ericnordelo left a comment

ericnordelo left a comment

ericnordelo left a comment

martriay Jan 25, 2023

andrew-fleming Jan 25, 2023 •

edited

Loading

Auto calculate max_fee #332

Auto calculate max_fee #332

Conversation

andrew-fleming commented Jan 5, 2023

ericnordelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-fleming Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

ericnordelo Jan 5, 2023 •

edited

Loading

ericnordelo Jan 6, 2023 •

edited

Loading

ericnordelo Jan 7, 2023 •

edited

Loading

andrew-fleming Jan 25, 2023 •

edited

Loading