Skip to content

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

Merged
merged 8 commits into from
Jun 12, 2025

Conversation

PatrickDinh
Copy link
Contributor

@PatrickDinh PatrickDinh commented Jun 10, 2025

Proposed Changes

class SendParams(TypedDict, total=False):
    """Parameters for sending a transaction"""

    max_rounds_to_wait: int | None
    suppress_log: bool | None
    populate_app_call_resources: bool | None
    cover_app_call_inner_transaction_fees: bool | None
  • The property that is related to this bug is cover_app_call_inner_transaction_fees. Previously, when calling the composer, the app deployer doesn't pass in the sendParams. 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.
  • To explain the bug [algokit bug] App Factory deploy method fails to auto-increase requested fee amount via simulate if the min_fee value from suggested params was changed and cached #169: (keep in mind that when the bug is reported, despite returning 50000 micro Algo min_fee, testnet still only require 1000 micro Algo per transaction) when the user uses the default min_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.
  • Add test to cover the above scenario.

@neilcampbell neilcampbell force-pushed the bug/169-deploy-send-params branch from 99d8810 to f7ad994 Compare June 10, 2025 11:14
Copy link

github-actions bot commented Jun 10, 2025

Coverage

Coverage Report
FileStmtsMissCoverMissing
src/algokit_utils
   _debugging.py1411887%21, 40–42, 45, 54, 62, 81, 87, 96, 110–114, 133, 141–143
   account.py330%1–12
   algorand.py981288%64–65, 79–80, 110–111, 138–139, 299, 322, 338, 357
   application_client.py330%1–11
   application_specification.py770%1–39
   config.py782963%23, 30–34, 73–74, 90, 95, 100, 111–116, 140–156
   deploy.py330%1–10
   logic_error.py330%1–10
src/algokit_utils/_legacy_v2
   _ensure_funded.py72199%96
   _transfer.py72396%14, 77–78
   account.py931386%15–18, 71–75, 109, 126, 156, 159, 203
   application_client.py5377786%56–57, 176, 181, 210, 320, 325–326, 328, 330, 799, 814, 832–835, 929, 969, 981, 994, 1036, 1096–1102, 1106–1111, 1113, 1149, 1156, 1269, 1305, 1319, 1357–1359, 1361, 1371–1428, 1439–1444, 1464–1467
   asset.py80495%24–27
   common.py13192%13
   deploy.py4072295%32–35, 170, 174–175, 193, 249, 339–340, 361, 395, 406–414, 429, 437, 593–594, 618
   network_clients.py73593%79–80, 105–106, 139
src/algokit_utils/accounts
   account_manager.py2554084%167, 183–184, 211–216, 295–305, 322, 339–344, 391–393, 422, 461, 481–485, 498, 557, 578, 735, 841, 920, 925, 940–941, 964
   kmd_account_manager.py731086%47–53, 94, 150, 157
src/algokit_utils/applications
   abi.py1397149%14, 75, 111, 117, 119, 121, 125–126, 141–160, 176, 179–185, 201–214, 227–239, 254–265
   app_client.py74221471%66–74, 133, 141, 328–331, 334, 337, 340, 343, 355–358, 361, 364, 367, 370, 382, 391, 400, 403–470, 483–541, 565–567, 584–587, 595–598, 606–609, 617–620, 628–631, 642–645, 658, 754–757, 791, 803, 815, 827, 839, 854, 869, 879, 889, 899, 909, 919, 956–969, 985, 1002, 1019, 1036, 1057, 1078, 1168, 1364, 1428–1429, 1478, 1480, 1486, 1539–1547, 1580–1583, 1586–1589, 1610, 1658, 1721–1723, 1736–1743, 1788, 1805, 1807, 1810, 1814, 1961, 1972–1973, 2001, 2011–2013, 2016–2038, 2048, 2057–2072, 2077–2082
   app_deployer.py2373784%96, 270, 277, 287–292, 295–299, 375–376, 587, 599–613, 625, 630–633, 642, 656, 663, 670, 699, 710–751
   app_factory.py2712790%430, 451, 460, 652, 662, 712, 944, 958–963, 974–975, 1008, 1046, 1064–1065, 1108, 1116–1129
   app_manager.py2231892%258, 279–280, 350–351, 396–401, 433, 457–458, 488, 516–519, 551, 560
src/algokit_utils/applications/app_spec
   arc32.py95892%198–207
   arc56.py5783294%71, 171, 292, 308–309, 379, 399–401, 453, 462, 464, 736, 750, 909, 911, 944–955, 968, 970–971, 987
src/algokit_utils/assets
   asset_manager.py1281291%282–283, 292, 298–322, 332
src/algokit_utils/clients
   _algokit_core_bridge.py21195%11
   client_manager.py1825570%25–27, 80–87, 112, 124, 149–151, 211, 222–225, 250, 289, 324–329, 364–366, 399, 416, 438, 483–486, 526–529, 570–573, 610–613, 635–660, 698, 711, 723
   dispenser_api_client.py961288%134–135, 139–142, 198–200, 219–221
src/algokit_utils/errors
   logic_error.py561180%14, 105–121
src/algokit_utils/models
   account.py921584%32, 34, 80–81, 128, 136, 144, 168–175, 188, 196, 209, 217
   amount.py1061784%35, 42, 88, 98, 104, 112, 117–119, 126, 131–133, 140, 147, 160, 166
   application.py66198%8
   state.py41588%61, 65–68
   transaction.py51394%66, 86, 91
src/algokit_utils/protocols
   account.py11282%17, 22
   typed_clients.py24483%12–24
src/algokit_utils/transactions
   _algokit_core_bridge.py16194%32
   transaction_composer.py111511390%38–45, 578, 623, 626, 631, 635–646, 678, 719, 786, 793, 815, 854, 951–980, 1009, 1027–1029, 1035–1050, 1055, 1063, 1065, 1067, 1083, 1089, 1127, 1135, 1148, 1244, 1294, 1743–1744, 1783–1784, 1825, 1867, 1878, 2048, 2051, 2070, 2075, 2099–2101, 2138–2174, 2181–2188, 2271, 2332, 2336, 2467, 2469–2470, 2520–2521
   transaction_creator.py75791%394, 427, 482, 520, 557, 590, 688
   transaction_sender.py1611094%99, 268, 311–312, 710–715, 720–721, 886
TOTAL676793086% 

Tests Skipped Failures Errors Time
406 0 💤 0 ❌ 0 🔥 5m 51s ⏱️

@neilcampbell neilcampbell force-pushed the bug/169-deploy-send-params branch from 799a3f4 to 558f5cb Compare June 10, 2025 11:53
@lempira lempira self-requested a review June 11, 2025 18:10
Copy link
Contributor

@lempira lempira left a 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"
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

@lempira lempira self-requested a review June 12, 2025 03:07
@PatrickDinh PatrickDinh merged commit fbf25ba into main Jun 12, 2025
4 checks passed
@PatrickDinh PatrickDinh deleted the bug/169-deploy-send-params branch June 12, 2025 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants