Skip to content

Conversation

@plasmatech8
Copy link

I do not know what peoples thoughts are about this is since I did not receive a response from #284.

This PR changes the fee calculations for Transaction classes under future.transaction so that it uses sp.min_fee instead of constants.min_txn_fee in calculations when flat_fee=false.

Rationale:

  • It is more intuitive to use min_fee in the SuggestedParams which the user is actively passing into the Transaction object, rather than a hidden/hard-coded value.
  • Application calls with inner transactions require a higher min-transaction fee. It is more convenient to set the value suggested_params.min_fee *= 2 rather than configuring flat fees.
  • The min-fee might change in the future, and it is better to use the value returned from the Algorand REST API than a hard-coded constant.

I tried running tests using make docker-test but it is excruciatingly slow for some reason, so I will think about that another time.

@CLAassistant
Copy link

CLAassistant commented Mar 6, 2022

CLA assistant check
All committers have signed the CLA.

@algochoi
Copy link
Contributor

Thanks for your patience - I think the SuggestedParameters may or may not have the min_fee field, and it's causing our current tests to fail:

 File "/py-algorand-sdk/algosdk/future/transaction.py", line 369, in __init__
    self.estimate_size() * self.fee, sp.min_fee
TypeError: '>' not supported between instances of 'NoneType' and 'int'

@jannotti
Copy link
Contributor

jannotti commented Jan 20, 2024

I agree, we should not use constants.min_txn_fee when we have better information available. Ideally, we'd some day be able to remove the constant entirely. If you want to rework this (there's currently a conflict), I help see through to merge. If not, I'll try to get the same change in soon.

@jannotti
Copy link
Contributor

It'll go in in #530

@jannotti jannotti closed this Jan 22, 2024
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.

4 participants