-
Notifications
You must be signed in to change notification settings - Fork 12
Fix bug where the app deployer doesn't pass send params into the composer #172
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
Conversation
99d8810
to
f7ad994
Compare
799a3f4
to
558f5cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put more information into the PR description on how this fixes the bug?
pyproject.toml
Outdated
@@ -11,6 +11,7 @@ python = "^3.10" | |||
py-algorand-sdk = "^2.4.0" | |||
httpx = ">=0.23.1,<=0.28.1" | |||
typing-extensions = ">=4.6.0" | |||
requests = ">=2.32.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this related to the PR? If so, can you explain how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeh ... not directly related to this bug but it's to fix an audit issue. Dependabot open this PR for the same issue too.
A question from me because I'm new to this project:
What is the normal procedure to upgrade transient dependencies in utils-py
?
- Method 1: what I did here, adding them to the list of dependencies in pyproject.toml file
- Method 2: what dependabot did, upgrade them in poetry.lock file
I'm happy to revert this change if we merge the dependabot's PR first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would say method 2 is the better approach. It's better not to add the package if the lib doesn't directly depend on it.
I would suggest doing what you suggested of reverting and merging the dependabot PR first. You can also revert the requests
install change and do a poetry update requests
. That should only update transitive deps in the lock file. You can see which packages depend on requests with poetry show requests
…arams # Conflicts: # poetry.lock
Proposed Changes
TransactionComposer.send
method tasks an optionalSendParams
parameter. This param defines the behaviour of the composer when sending the transaction group.cover_app_call_inner_transaction_fees
. Previously, when calling the composer, the app deployer doesn't pass in thesendParams
. Therefore,cover_app_call_inner_transaction_fees
is false and the composer would skip this step completely. As a result, inner transaction fees aren't counted for in app deploy ABI methods.min_fee
, testnet still only require 1000 micro Algo per transaction) when the user uses the defaultmin_fee
from testnet of 50000 micro Algo, the transaction group was sent successfully because 50000 is plenty for the delete transaction and all the inners. When they updated (and cached) the suggested params to 1000 micro Algo, the delete transaction was sent with fee 1000 micro Algo, this isn't enough to cover the inner transactions.