Sending ABI methods with composer #221
Conversation
PatrickDinh
force-pushed
the
feat/app-client
branch
from
133f4db to
9654b6d
Compare
| @@ -152,3 +152,60 @@ pub struct AssetDestroyParams { | |||
| /// ID of the existing asset to be destroyed. | |||
| pub asset_id: u64, | |||
| } | |||
|
|
|||
| pub fn build_asset_create(params: &AssetCreateParams, header: TransactionHeader) -> Transaction { | |||
There was a problem hiding this comment.
these methods were refactored out of composer.rs
There was a problem hiding this comment.
Pull Request Overview
This PR implements the ability to send ABI methods using the composer, introducing comprehensive support for application method calls alongside existing raw transaction functionality.
- Adds new
ApplicationMethodCallParamsand related parameter structures for ABI method calls - Implements ABI method argument handling including payments, application calls, transactions, and references
- Extends the composer with
add_application_method_callfunctionality to complement existing raw transaction methods
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| inner_fee_coverage.rs | Refactors test file to use new ApplicationRawCallParams instead of deprecated ApplicationCallParams |
| asset_transfer.rs | Minor import reorganization moving asset-related imports to main utils module |
| application_call.rs | Major addition of comprehensive ABI method call tests and new parameter structures |
| contract.py | Adds new sandbox contract with various ABI methods for testing complex scenarios |
| Sandbox.arc56.json | Contract specification file defining the sandbox contract's ABI interface |
| payment.rs | Adds utility functions build_payment and build_account_close for transaction construction |
| mod.rs | Updates module exports to include new transaction parameter types and removes deprecated ones |
| key_registration.rs | Adds builder functions for different types of key registration transactions |
| @@ -26,3 +26,22 @@ pub struct AccountCloseParams { | |||
| /// The address to receive the remaining funds. | |||
| pub close_remainder_to: Address, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The new build_payment function lacks documentation. Add a doc comment explaining its purpose, parameters, and return value.
| /// Builds a payment transaction that sends ALGO from the sender to the specified receiver. | |
| /// | |
| /// # Parameters | |
| /// - `params`: The parameters specifying the receiver and amount to send. | |
| /// - `header`: The transaction header, including sender and other common fields. | |
| /// | |
| /// # Returns | |
| /// A `Transaction` representing the payment transaction. |
Copilot uses AI. Check for mistakes.
| close_remainder_to: None, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
The new build_account_close function lacks documentation. Add a doc comment explaining its purpose, parameters, and return value.
| /// Builds a payment transaction that closes an account by transferring all remaining funds | |
| /// to the specified `close_remainder_to` address. | |
| /// | |
| /// # Parameters | |
| /// - `params`: The parameters specifying the recipient of the remaining funds. | |
| /// - `header`: The transaction header, including the sender's address. | |
| /// | |
| /// # Returns | |
| /// A `Transaction` representing the account close operation. |
Copilot uses AI. Check for mistakes.
| @@ -21,3 +23,51 @@ pub struct OfflineKeyRegistrationParams { | |||
| pub struct NonParticipationKeyRegistrationParams { | |||
| pub common_params: CommonParams, | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The new build_online_key_registration function lacks documentation. Add a doc comment explaining its purpose, parameters, and return value.
| /// Builds an online key registration transaction. | |
| /// | |
| /// # Parameters | |
| /// - `params`: Reference to [`OnlineKeyRegistrationParams`] containing the key registration details, | |
| /// such as vote key, selection key, voting period, key dilution, and optional state proof key. | |
| /// - `header`: The [`TransactionHeader`] to use for the transaction. | |
| /// | |
| /// # Returns | |
| /// A [`Transaction`] representing an online key registration with the provided parameters. |
Copilot uses AI. Check for mistakes.
| non_participation: None, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
The new build_offline_key_registration function lacks documentation. Add a doc comment explaining its purpose, parameters, and return value.
| /// Builds an offline key registration transaction. | |
| /// | |
| /// # Parameters | |
| /// - `params`: The parameters for the offline key registration, including common transaction fields and optional non-participation flag. | |
| /// - `header`: The transaction header containing metadata such as sender, fee, and validity period. | |
| /// | |
| /// # Returns | |
| /// A `Transaction` representing the offline key registration. |
Copilot uses AI. Check for mistakes.
| non_participation: params.non_participation, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
The new build_non_participation_key_registration function lacks documentation. Add a doc comment explaining its purpose, parameters, and return value.
| /// Builds a key registration transaction that marks the account as non-participating. | |
| /// | |
| /// # Parameters | |
| /// - `_params`: Reference to `NonParticipationKeyRegistrationParams` containing common transaction parameters. | |
| /// - `header`: The transaction header to use for the transaction. | |
| /// | |
| /// # Returns | |
| /// A `Transaction` representing a non-participation key registration. |
Copilot uses AI. Check for mistakes.
PatrickDinh
changed the title
| }) | ||
| } | ||
|
|
||
| pub fn build_asset_reconfigure( |
There was a problem hiding this comment.
was there a reason we initially defined this as reconfigure instead of configure? We don't have term reconfigure on utils py or ts
There was a problem hiding this comment.
for the utils layer, we wanted to add some abstraction on top of asset configure, therefore, we have build_asset_create, build_asset_reconfigure, and build_asset_destroy. The different is small, for example, the create method doesn't need asset ID because it's always 0.
There was a problem hiding this comment.
In utils-ts/py we call it asset_config. Based on the previous decision to align to utils-ts/py naming wherever possible, we should rename this as part of the stabilisation.
| TransactionPlaceholder, | ||
| } | ||
|
|
||
| mod sealed { |
There was a problem hiding this comment.
Having a short comment describing why we have this sealed module would be useful. This pattern was new to me and other readers of the might also not be familiar with the purpose
| ABIMethodArgType::Value(abi_type) => abi_type.clone(), | ||
| // Reference and transaction types encoded as uint8 indexes | ||
| // unwrap() is used because 8 is a valid BitSize | ||
| ABIMethodArgType::Reference(_) => ABIType::Uint(BitSize::new(8).unwrap()), |
There was a problem hiding this comment.
I think it's generally preferable to use expect, with the arg being a "should" statement describing the invariant
| ABIMethodArgType::Reference(_) => ABIType::Uint(BitSize::new(8).unwrap()), | |
| ABIMethodArgType::Reference(_) => ABIType::Uint(BitSize::new(8).expect("8 should always be a valid BitSize")), |
| // Reference and transaction types encoded as uint8 indexes | ||
| // unwrap() is used because 8 is a valid BitSize | ||
| ABIMethodArgType::Reference(_) => ABIType::Uint(BitSize::new(8).unwrap()), | ||
| ABIMethodArgType::Transaction(_) => unreachable!(), // filtered out above |
There was a problem hiding this comment.
I think it'd be preferable to use filter_map and return None for this branch so we can avoid the uncreachable!
| Ok(ABIValue::Uint(BigUint::from(foreign_index))) | ||
| } | ||
| ProcessedAppMethodCallArg::ABIValue(value) => Ok(value.clone()), | ||
| ProcessedAppMethodCallArg::TransactionPlaceholder => unreachable!(), // filtered out above |
There was a problem hiding this comment.
Same as above, I think filter_map is preferable
PatrickDinh
force-pushed
the
feat/app-client
branch
from
50b464b to
78fb8cd
Compare
|
🎉 This PR is included in version 1.0.0-alpha.49 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0-alpha.41 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes #
Proposed Changes