Skip to content

feat: app manager; asset manager; creator and sender abstractions #229

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 56 commits into from
Aug 15, 2025
Merged

feat: app manager; asset manager; creator and sender abstractions #229

merged 56 commits into from
Aug 15, 2025

Conversation

@aorumbayev
Copy link
Collaborator

@aorumbayev aorumbayev commented Aug 13, 2025

Minimal versions of app manager; asset manager and full versions of creator and sender abstractions as prereqs for app client pr

This comment was marked as outdated.

Sign in to view

aorumbayev
aorumbayev commented Aug 13, 2025
View reviewed changes
aorumbayev
aorumbayev commented Aug 13, 2025
View reviewed changes
aorumbayev
aorumbayev commented Aug 13, 2025
View reviewed changes
aorumbayev
aorumbayev commented Aug 13, 2025
View reviewed changes
aorumbayev
aorumbayev commented Aug 13, 2025
View reviewed changes
@aorumbayev aorumbayev force-pushed the feat/app-client-prereqs branch from 1ccea65 to a336c4a Compare August 13, 2025 19:18
@aorumbayev aorumbayev force-pushed the feat/app-client-prereqs branch from a336c4a to 8e8b4ac Compare August 13, 2025 19:26
neilcampbell
neilcampbell reviewed Aug 14, 2025
View reviewed changes
neilcampbell
neilcampbell reviewed Aug 14, 2025
View reviewed changes
@aorumbayev
fix(address): replace get_application_address with Address::from_app_id
949782e 
- Remove duplicate get_application_address function from utils.rs
- Replace all usages with Address::from_app_id method from PR #225
- Update imports and function calls across codebase
- Add test for Address::from_app_id method
- Maintain backward compatibility for functionality

Addresses PR comment by neilcampbell about using Address trait approach
@aorumbayev
perf(clients): optimize algod client cloning with Arc<AlgodClient>
4cbc185 
- Update Composer to use Arc<AlgodClient> instead of owned AlgodClient
- Remove unnecessary dereference-then-clone patterns in AlgorandClient
- Fix test files to use Arc pattern correctly
- Eliminate 6 unnecessary AlgodClient clones in AlgorandClient constructor
- Improve memory efficiency by sharing client instances across components

Addresses PR comment by neilcampbell about excessive cloning
@aorumbayev
perf(app_manager): use SHA256 hash of TEAL code as cache key
bdd6b5e 
- Replace direct TEAL code string usage as cache key with SHA256 hash
- Add hash_teal_code() helper method for consistent hashing
- Update both compile_teal and get_compilation_result methods
- Improve memory efficiency for large TEAL programs
- Maintain O(1) cache lookup performance with fixed-size keys

Addresses PR comment by neilcampbell about efficient cache key usage
@aorumbayev
docs(app_manager): add docstrings to public struct fields
d6f2547 
- Add comprehensive documentation to AppInformation fields
- Add documentation to BoxName struct fields
- Add documentation to ParsedABIReturn struct fields
- Improve API documentation for public interfaces

Addresses PR comment by neilcampbell about documenting public fields
@aorumbayev
docs(algorand_client): add TODO for EmptySigner replacement
09c1614 
- Add TODO comments explaining EmptySigner is temporary
- Note that dynamic signer resolution will be implemented with AccountManager
- Reference TypeScript's getSigner function pattern as target behavior

Addresses PR comment by neilcampbell about signer behavior
@aorumbayev
fix(app_manager): change BoxIdentifier to Vec<u8> for binary data sup…
36b037f 
…port

- Change BoxIdentifier type from String to Vec<u8>
- Add documentation explaining binary nature of box identifiers
- Update get_box_reference method to handle Vec<u8> directly
- Add test for binary box identifier handling
- Ensure proper support for non-UTF-8 box names

Addresses PR comment by neilcampbell about box identifier type correctness
@aorumbayev
refactor(validation): remove duplicate validation and use transact crate
d8639d9 
- Remove duplicate asset creation validation from utils sender.rs
- Remove duplicate application call validation from utils sender.rs
- Update build_asset_create to use AssetConfigTransactionBuilder with validation
- Update build_app_call to use ApplicationCallTransactionBuilder with validation
- Update composers to handle validation error results properly
- Add comprehensive tests for validation error flow
- Maintain intentional early validation for payment/asset transfer amounts

Addresses PR comment by neilcampbell about leveraging transact crate validation
@aorumbayev
fix(abi): align ABIReturn type usage across crates
9605a24 
- Move ABIReturn struct to algokit_abi crate for consistency
- Replace ParsedABIReturn with standard ABIReturn throughout codebase
- Update method signatures to use Result<Option<ABIReturn>, AppManagerError>
- Remove duplicate ABI type definitions
- Centralize all ABI types in algokit_abi crate

Addresses PR comment #2275622880 about ABI type consistency
@aorumbayev
fix(asset): make close_remainder_to non-optional for opt-out consistency
a1fe4e8 
- Update AssetOptOutParams to require creator field (remove Option wrapper)
- Fix build_asset_opt_out to set close_remainder_to to creator address
- Add convenience method asset_opt_out_with_auto_creator for backward compatibility
- Update all tests to provide explicit creator addresses
- Align Rust behavior with TypeScript/Python implementations

Addresses PR comment #2275906304 about type consistency
@aorumbayev
feat(app-manager): add ABIMethod-based box value decoding for TS comp…
bf5dfd9 
…atibility

- Add get_box_value_from_abi_method and get_box_values_from_abi_method methods
- Use ABIMethod parameter instead of ABIType for TS alignment
- Return ABIReturn objects instead of just ABIValue for consistency
- Maintain backward compatibility with existing ABIType methods
- Enable integration with ABIReturn-based workflows from Task 6

Addresses PR comment #2275629959 about using ABI method instead of ABIType
@aorumbayev
refactor(app-manager): use Address::from_app_id helper
e6cdbf7 
- Replace manual address generation with Address::from_app_id helper
- Remove duplicate get_app_address method
- Remove unused Sha512_256 import
- Maintain consistency with existing Address trait usage pattern

Addresses PR comment #2275633627 about using the Address helper function
@aorumbayev
fix(app-manager): use Vec<u8> keys for app state consistency with TS
9ea120e 
- Change HashMap keys from String to Vec<u8> in app state structures
- Use key_raw (binary data) as HashMap key instead of string representation
- Add comprehensive test coverage for binary key handling
- Align with TypeScript UInt8Array typing for cross-language consistency
- Ensure proper binary data support for non-UTF-8 keys

Addresses PR comment #2275729899 about key field deserialization
@aorumbayev
fix(teal-value): change bytes field to Vec<u8> for TS consistency
81d52e2 
- Update TealValue.bytes from String to Vec<u8> in algod and indexer clients
- Add automatic base64 deserialization using serde_with::base64::Base64
- Remove manual base64 decoding in app_manager since serde handles it automatically
- Update test coverage for bytes type handling with binary data
- Align with TypeScript UInt8Array typing for cross-language consistency

Addresses PR comment #2275733533 about bytes field deserialization
@aorumbayev
refactor(composer): remove duplicate From<SendParams> implementation
37990da 
- Remove unrelated duplicate impl From<SendParams> for BuildParams
- Keep original impl From<&SendParams> for BuildParams which is actually used
- Clean up code organization per Neil's feedback
- This duplicate belonged in inner fee coverage PR, not this one

Addresses PR comment #2275909098 about unrelated addition
@aorumbayev
refactor(creator): rename asset_reconfigure to asset_config
d33af71 
- Rename asset_reconfigure method to asset_config for better consistency
- Align with Algorand protocol terminology and other language implementations
- Remove TODO comment that requested this rename
- Improve public API clarity while maintaining internal implementation

Addresses PR comment #2275990103 about function renaming
@aorumbayev
refactor(sender): rename asset_reconfigure to asset_config
03c18d7 
- Rename asset_reconfigure method to asset_config for consistency
- Update function documentation comment accordingly
- Align with Task 21 changes in creator.rs for API consistency
- Maintain same parameter types and internal implementation

Addresses PR comment #2276017888 about companion function renaming
@aorumbayev
fix(sender): return program bytes for both create and update operations
8169d89 
- Add compiled_approval and compiled_clear to SendAppCreateResult
- Extract compilation metadata in both app_create and app_create_method
- Align Rust behavior with TypeScript/Python implementations
- Ensure consistent program bytes return across all app operations
- Maintain backward compatibility with additive changes

Addresses PR comment #2276031097 about program bytes return behavior
@aorumbayev
feat(asset-manager): replace custom types with raw algod client types
0f6ac9c 
- Remove custom AssetInformation struct in favor of algod_client::models::Asset
- Remove custom AccountAssetInformation struct in favor of algod_client::models::AccountAssetInformation
- Leverage generator investment: field renaming and base64 deserialization already implemented in algod types
- Breaking change: get_by_id() now returns Asset instead of AssetInformation
- Breaking change: get_account_information() now returns AccountAssetInformation instead of custom struct
- Update all usage sites to work with nested algod type structure (asset.params.*, account_info.asset_holding.*)
- Maintain zero functionality loss while eliminating duplicate type definitions
- Architecture improvement: direct use of high-quality generated types instead of wrapper abstraction
- Aligns with modular building blocks vision - leverage algod client types directly
@aorumbayev
fix(asset-opt-out): rename creator field to close_remainder_to
efbdfce 
- Rename AssetOptOutParams.creator → AssetOptOutParams.close_remainder_to
- Address Neil's feedback: 'close_remainder_to does feel more correct to me'
- Aligns with TypeScript/Python implementations and Algorand protocol semantics
- Breaking change: users now explicitly specify close remainder destination
- Updated all test usages to use new field name
- No functional changes - same validation and transaction building logic
- Improves API clarity: users decide where to close remainder funds
@aorumbayev
chore: sanity tweaks
920bd7c
@aorumbayev aorumbayev force-pushed the feat/app-client-prereqs branch from 36788bc to 920bd7c Compare August 14, 2025 21:44
@aorumbayev aorumbayev requested a review from Copilot August 14, 2025 21:59
Copilot AI reviewed Aug 14, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces minimal versions of the AppManager and AssetManager clients, along with complete implementations of the TransactionSender and TransactionCreator abstractions. These components serve as essential prerequisites for the upcoming app client functionality by providing TEAL compilation, asset management, and enhanced transaction orchestration capabilities.

  • Adds minimal AppManager with TEAL compilation, caching, and template processing capabilities
  • Adds minimal AssetManager for asset information retrieval and bulk opt-in/opt-out operations
  • Implements complete TransactionSender with validation, result processing, and manager coordination
  • Implements complete TransactionCreator for building individual transactions with enhanced parameter validation

Reviewed Changes

Copilot reviewed 48 out of 48 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
crates/indexer_client/src/models/teal_value.rs Updates TealValue to use Vec for bytes field with base64 serde
crates/algokit_utils/tests/transactions/sender.rs Comprehensive test suite for TransactionSender functionality
crates/algokit_utils/tests/transactions/creator.rs Comprehensive test suite for TransactionCreator functionality
crates/algokit_utils/tests/clients/asset_manager.rs Test suite for AssetManager operations
crates/algokit_utils/tests/clients/app_manager.rs Test suite for AppManager TEAL compilation and template processing
crates/algokit_utils/tests/clients/algorand_client.rs Test suite for AlgorandClient initialization and integration
crates/algokit_utils/src/transactions/sender.rs TransactionSender implementation with validation and result processing
crates/algokit_utils/src/transactions/creator.rs TransactionCreator implementation for building individual transactions
crates/algokit_utils/src/transactions/sender_results.rs Rich result types for transaction sending operations
crates/algokit_utils/src/clients/asset_manager.rs AssetManager implementation for asset operations
crates/algokit_utils/src/clients/app_manager.rs AppManager implementation for TEAL compilation and app state management
crates/algokit_utils/src/clients/algorand_client.rs Enhanced AlgorandClient with manager integrations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@aorumbayev
refactor: improve test focus and remove implementation detail comments
153c79f 
- Remove internal cache method access from tests, focus on behavior validation
- Simplify test comments to avoid over-explanation of obvious operations
- Remove TypeScript/Python alignment mentions from API documentation
- Improve test maintainability by testing public interface rather than internals
neilcampbell
neilcampbell reviewed Aug 15, 2025
View reviewed changes
crates/algokit_transact/src/transaction_ext.rs Outdated
/// Extension trait providing accessor methods for Transaction enum variants
pub trait TransactionExt {
/// Returns true if this is a payment transaction
fn is_payment(&self) -> bool;
Copy link
Collaborator

@neilcampbell neilcampbell Aug 15, 2025

Choose a reason for hiding this comment

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

I'm also wondering it we'd be better to simply have a function which exposes a transaction type enum defined in algokit_transact (I think we'd need to add).

Copy link
Collaborator Author

@aorumbayev aorumbayev Aug 15, 2025

Choose a reason for hiding this comment

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

so i still kept the transaction ext trai with is methods, they are useful for single type checks and simple conditionals, loosely mirroring the approach with wrapped transaction classes we used in utils py in relation to sender abstraction. But also added a TransactionType enum for cases when more comprehensive pattern matching is needed.

Copy link
Collaborator

@neilcampbell neilcampbell Aug 15, 2025

Choose a reason for hiding this comment

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

I figured if we had transaction type we could remove all the is_{type} methods.

@aorumbayev
fix(sender): rename app method functions with _method_call suffix
543eb69 
- app_call_method → app_call_method_call
- app_create_method → app_create_method_call
- app_update_method → app_update_method_call
- app_delete_method → app_delete_method_call

Addresses PR comments about naming consistency and explicitness.
Updated test usage to match new method names.

Refs: PR#229 comment IDs 2278470125, 2278475769, 2278475920, 2278476947
@aorumbayev
feat(app_manager): add ABIType-based box value methods for API consis…
1b737f7 
…tency

- Add get_box_value_from_abi_type() returning ABIValue directly
- Add get_box_values_from_abi_type() returning Vec<ABIValue>
- Keep existing ABIMethod-based methods returning ABIReturn
- Provides both simple direct decoding and rich context workflows
- Matches TypeScript/Python API patterns while maintaining workflow integration

Addresses PR comments about API consistency and providing both method types.
Tests added to demonstrate the differences between approaches.

Refs: PR#229 comment IDs 2278419350, 2278420074, 2278459006
@aorumbayev
fix(consistency): address naming, visibility, and documentation issues
ee9a198 
- Rename 'base' field to 'common_params' in SendResult structs for consistency
- Remove public parse_abi_return method, use static get_abi_return instead
- Update base64 encoding comment to specify JSON responses over HTTP
- Aligns with established codebase patterns (common_params used 74+ times)
- Reduces public API surface while maintaining functionality

Addresses PR comments about consistency and precision.

Refs: PR#229 comment IDs 2278487146, 2278420725, 2278425272
@aorumbayev
refactor(transact): move TransactionExt trait to mod.rs for better or…
d10f907 
…ganization

- Move TransactionExt trait from standalone file to transactions/mod.rs
- Co-locate transaction extension logic with Transaction type definition
- Update imports to maintain public API compatibility
- Remove transaction_ext.rs file (no longer needed)
- All tests passing, maintains backward compatibility

Improves code organization by grouping transaction-related functionality.

Refs: PR#229 comment ID 2278400067
@aorumbayev
feat(tests): improve asset test ergonomics with convenience methods
50d944e 
- Add asset_id() method as semantic alias for index field
- Add convenience methods to flatten .params. access pattern:
  total(), decimals(), unit_name(), name()
- Update tests to use ergonomic methods instead of nested field access
- Maintains raw algod types while improving developer experience
- Addresses test structure and naming consistency feedback

Refs: PR#229 comment IDs 2278463115, 2278464299
@aorumbayev
feat(asset): simplify opt-out API with optional close_remainder_to
a70b82d 
- Make close_remainder_to field optional in AssetOptOutParams
- Auto-resolve to asset creator when None (most common use case)
- Maintain explicit address option with Some(address)
- Update all usage and tests to use new optional pattern
- Eliminates need for separate convenience methods
- Cleaner API design as suggested in review

Users can now pass None for automatic creator resolution or
Some(address) for explicit control.

Refs: PR#229 comment ID 2278439775
@aorumbayev
feat(transact): add TransactionType enum for ergonomic type identific…
16a8624 
…ation

- Add TransactionType enum with variants: Payment, AssetTransfer, AssetCreate,
  AssetFreeze, ApplicationCall, KeyRegistration
- Add transaction_type() method to TransactionExt trait
- Enables ergonomic pattern matching: match tx.transaction_type() { ... }
- Maintains backward compatibility with existing is_*() methods
- Zero-cost abstraction with comprehensive test coverage
- Addresses architectural feedback about unified transaction type approach

Users can now choose between individual boolean methods or
enum-based pattern matching for transaction type handling.

Refs: PR#229 comment ID 2278408184
@aorumbayev
refactor: remove unnecessary backwards compatibility within dev branch
64fa7e4 
- Remove backwards compatibility test for is_*() vs transaction_type() methods
  Both approaches were added in same dev branch, no need for compatibility testing
- Remove convenience methods from algod Asset model added just for test ergonomics
  Use direct field access in tests instead of bloating generated API types
- Embrace refined approaches rather than maintaining compatibility with interim code

This cleanup eliminates 32 lines of unnecessary code while maintaining
the same functionality. Generated API types should stay lean.
@aorumbayev
feat(asset-manager): restore AssetInformation type with flattened erg…
465ec52 
…onomic interface

- Add AssetInformation struct with direct field access (asset.total vs asset.params.total)
- Remove nested .params access patterns for better developer experience
- Implement automatic conversion from algod_client::models::Asset
- Update AssetManager.get_by_id() to return AssetInformation instead of raw Asset
- Update all consuming code (sender.rs, tests) to use flattened structure
- Add comprehensive unit tests for AssetInformation conversion
- Export AssetInformation type from crate for public usage
- Align with TypeScript/Python AlgoKit implementations for consistency

Breaking change: AssetManager.get_by_id() now returns AssetInformation
instead of algod_client::models::Asset, but provides better ergonomics.

Addresses PR comment about folding away nested .params structure
neilcampbell
neilcampbell approved these changes Aug 15, 2025
View reviewed changes
crates/algokit_transact/src/transaction_ext.rs Outdated
/// Extension trait providing accessor methods for Transaction enum variants
pub trait TransactionExt {
/// Returns true if this is a payment transaction
fn is_payment(&self) -> bool;
Copy link
Collaborator

@neilcampbell neilcampbell Aug 15, 2025

Choose a reason for hiding this comment

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

I figured if we had transaction type we could remove all the is_{type} methods.

aorumbayev added a commit that referenced this pull request Aug 15, 2025
@aorumbayev
refactor: remove ABIMethod-based box value methods
c8d7ae1 
Removes get_box_value_from_abi_method() and get_box_values_from_abi_method()
methods that don't make architectural sense:

- Box storage is independent of method calls
- ABIMethod represents function calls, not data storage formats
- Box data should be decoded by storage type (ABIType), not method signatures

This addresses Neil's feedback that it doesn't make sense to pass an ABI
method to functions reading box data. The TypeScript implementation confirms
this approach - it only uses ABIType for box decoding, never ABIMethod.

Maintains correct methods:
- get_box_value_from_abi_type() ✓ (TypeScript-compatible)
- get_abi_return() taking ABIMethod ✓ (for transaction returns)

Fixes #229 review comment
@aorumbayev
refactor: remove ABIMethod-based box value methods
a5c2ff2 
Removes get_box_value_from_abi_method() and get_box_values_from_abi_method()
methods that don't make architectural sense:

- Box storage is independent of method calls
- ABIMethod represents function calls, not data storage formats
- Box data should be decoded by storage type (ABIType), not method signatures

This addresses Neil's feedback that it doesn't make sense to pass an ABI
method to functions reading box data. The TypeScript implementation confirms
this approach - it only uses ABIType for box decoding, never ABIMethod.

Maintains correct methods:
- get_box_value_from_abi_type() ✓ (TypeScript-compatible)
- get_abi_return() taking ABIMethod ✓ (for transaction returns)

Fixes #229 review comment
@aorumbayev aorumbayev force-pushed the feat/app-client-prereqs branch from c8d7ae1 to a5c2ff2 Compare August 15, 2025 14:29
@aorumbayev
refactor: avoid mutable parameter in asset_opt_out method
b60f626 
- Remove mut from AssetOptOutParams parameter
- Use conditional assignment with struct reconstruction instead of mutation
- Addresses Neil's PR feedback to avoid unnecessary mutability
aorumbayev
aorumbayev commented Aug 15, 2025
View reviewed changes
crates/algokit_utils/src/transactions/sender_results.rs
/// Get the asset configuration transaction from the common transaction
pub fn asset_config_transaction(&self) -> Option<&AssetConfigTransactionFields> {
self.common_params.transaction.as_asset_create()
if let Transaction::AssetConfig(asset_config) = &self.common_params.transaction {
Copy link
Collaborator Author

@aorumbayev aorumbayev Aug 15, 2025

Choose a reason for hiding this comment

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

@neilcampbell ended up removing the enum and transaction ext trait in favor of idiomatic pattern matching

@aorumbayev aorumbayev force-pushed the feat/app-client-prereqs branch from da2f57f to 5ae6826 Compare August 15, 2025 15:01
@aorumbayev
refactor: remove TransactionExt trait and TransactionType enum, embra…
86f32a3 
…ce idiomatic Rust

Removes abstraction layers that don't add value and encourages idiomatic Rust patterns:

REMOVED:
- TransactionExt trait (68 lines of abstraction)
- TransactionType enum (26 lines)
- All is_*() boolean methods (is_payment, is_asset_transfer, etc.)
- All as_*() accessor methods except as_app_call() (the only one actually used)
- transaction_type() method

KEPT (Essential methods actually used in codebase):
- Header accessors: sender(), fee(), first_valid_round(), last_valid_round(), note()
- as_app_call() - the only accessor method actually used in production

MIGRATION:
- sender_results.rs: Updated to use matches!(tx, Transaction::Payment(_)) instead of tx.is_payment()
- Tests: Demonstrate idiomatic pattern matching instead of abstraction layers

BENEFITS:
- 90% reduction in API surface area (from 18 methods to 6)
- Zero runtime overhead (direct pattern matching vs trait dispatch)
- Idiomatic Rust - follows ecosystem patterns like Option/Result
- Matches TypeScript/Python approach of direct type checking
- Encourages explicit, type-safe pattern matching
@aorumbayev aorumbayev force-pushed the feat/app-client-prereqs branch from 5ae6826 to 86f32a3 Compare August 15, 2025 15:34
@aorumbayev aorumbayev merged commit 477a73c into main Aug 15, 2025
19 checks passed
@aorumbayev aorumbayev deleted the feat/app-client-prereqs branch August 15, 2025 15:45
@engineering-ci
Copy link
Contributor

engineering-ci bot commented Aug 15, 2025

🎉 This PR is included in version 1.0.0-alpha.53 🎉

The release is available on:

Your semantic-release bot 📦🚀

@engineering-ci
Copy link
Contributor

engineering-ci bot commented Aug 15, 2025

🎉 This PR is included in version 1.0.0-alpha.45 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@neilcampbell neilcampbell neilcampbell approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aorumbayev @neilcampbell