feat: Deprecate transfer and asset modules #291

robdmoore · 2024-06-17T17:24:41Z

Follows on from #287.

Breaking changes:

Renamed AlgokitComposer to AlgoKitComposer to match AlgoKit naming conventions
Collapsed the second object in algorand.send.type(params, executeOptions) to combine into params and make it easier to use based on devrel feedback
Order of algorand.account.rekeyed() parameters to be rekeyed(sender, signer) since it conceptually makes more sense (the sender is rekeyed so should come first)
All microAlgo return values from algorand.account.getInformation() now return an AlgoAmount, renamed amount to balance and round to validAsAtRound (which is now a bigint for broader consistency)
Renamed algorand.account.getAssetInformation to algorand.asset.getAccountInformation

Added:

Return value of algorand.send.assetCreate now includes { assetId: bigint }
Up to date documentation for rekeyAccount, AlgorandClient, asset, dispenser client, indexer, testing, transfer
Up to date documentation for how to refer to an Algo amount (Algo (not plural) in general and ALGO (not plural) when referring to a specific amount)
AssetManager class and algorand.asset.getById(), algorand.asset.bulkOptIn, and algorand.asset.bulkOptOut
indexer export off of @algorandfoundation/algokit-utils as the future home of all indexer methods
Added algorand.client.getTestNetDispenserFromEnvironment
Added algorand.account.assetBulkOptIn
Added algorand.account.assetBulkOptOut
Added algorand.account.ensureFunded
Added algorand.account.ensureFundedFromEnvironment
Added algorand.account.ensureFundedFromTestNetDispenserApi
Added algorand.account.rekeyAccount
Added algorand.send/transaction.assetOptOut
Added buildTransactions method to AlgoKitComposer so you can build transactions without needing to register a signer
Added .algo and .microAlgo methods/properties in place of the plural version for AlgoAmount to reflect the current guidance on how to represent Algo amounts, kept previous versions of those methods for now to avoid the breaking change

Deprecated the following in favour of AlgorandClient functionality:

algokit.createAsset
algokit.assetOptIn
algokit.assetOptOut
algokit.assetBulkOptIn
algokit.assetBulkOptOut
algokit.ensureFunded
algokit.transferAsset
algokit.rekeyAccount
algokit.transferAlgos

docs/capabilities/algorand-client.md

robdmoore · 2024-06-17T17:26:36Z

src/account/account.ts

@@ -144,7 +150,26 @@ export function getAccountAddressAsString(addressEncodedInB64: string): string {
 * @returns The account information
 */
 export async function getAccountInformation(sender: string | SendTransactionFrom, algod: Algodv2): Promise<AccountInformation> {
-  return new AccountManager(new ClientManager({ algod })).getInformation(getSenderAddress(sender))
+  const account = AccountInformationModel.from_obj_for_encoding(await algod.accountInformation(getSenderAddress(sender)).do())


Keeping backwards compatibility in the legacy method (made changes to this method in AccountManager to return better types (AlgoAmount rather than number).

robdmoore · 2024-06-17T17:28:42Z

src/index.ts


 // Up to date exports
 export * from './amount'
 export * from './config'
+export * as indexer from './indexer-lookup'


I think the indexer methods are useful as-is. Per previous discussions we don't want to make them that easy to use given preference to discourage use of indexer. I figure exposing them through an indexer export off the / is a good way of exposing them without making for a noisy / import.

robdmoore · 2024-06-17T17:29:13Z

src/testing/_asset.ts


-export async function generateTestAsset(client: Algodv2, sender: Account, total?: number) {
+export async function generateTestAsset(algorand: AlgorandClient, sender: string, total?: number) {


Internal method so it's a non-breaking change for the library.

robdmoore · 2024-06-17T17:31:04Z

src/types/algorand-client.asset.spec.ts

@@ -0,0 +1,198 @@
+import { describe, test } from '@jest/globals'


Rename of asset.spec.ts with changes to use algorandClient

robdmoore · 2024-06-17T17:31:37Z

src/types/algorand-client.transfer.spec.ts

@@ -0,0 +1,378 @@
+import { describe, test } from '@jest/globals'


Rename of transfer.spec.ts with changes to use algorandClient

robdmoore · 2024-06-17T17:32:35Z

src/types/algorand-client.ts

@@ -118,8 +135,13 @@ export class AlgorandClient {
    return this._accountManager
  }

+  /** Methods for interacting with assets. */
+  public get asset() {
+    return this._assetManager


Added new manager (assetManager). For now it's simple (single method), but over time it may grow in complexity.

Should the bulk functions go in the AssetManager?

robdmoore · 2024-06-17T17:32:55Z

src/types/algorand-client.ts

@@ -134,19 +156,19 @@ export class AlgorandClient {
      preLog?: (params: T, transaction: Transaction) => string
      postLog?: (params: T, result: SendSingleTransactionResult) => string
    },
-  ): (params: T, config?: ExecuteParams) => Promise<SendSingleTransactionResult> {
-    return async (params, config) => {
+  ): (params: T & ExecuteParams) => Promise<SendSingleTransactionResult> {


Combining these together per devrel feedback

joe-p · 2024-06-17T17:58:54Z

Haven't done a proper review yet, but not sure how I feel about algorand.send/transaction.rekey. On one hand it's nice to have such an important operation be so explicit. On the other hand, I'm worried it might give developers the incorrect impression that a rekey is a special transaction type.

robdmoore · 2024-06-17T18:02:11Z

No different to opt-in/opt-out imo

robdmoore · 2024-06-17T18:04:50Z

Also allows for a really explicit approach to such a dangerous but useful operation with a minimal set of parameters with intellisense. we could illustrate in the method description that rekey can also be added to any other transaction by setting the optional rekeyTo value?

joe-p · 2024-06-17T18:06:36Z

No different to opt-in/opt-out imo

To me the main difference is that there are no negative consequences of a developer thinking optin/out is a seperate tx type. With rekeys, however, if a developer thinks that rekey is a special transaction type and reads code somewhere with a payment transaction or appl, they might be oblivious to the fact that it's rekeying an account.

Essentially I'm nervous it might "train" developers to look for .rekey calls rather than rekeyTo fields

robdmoore · 2024-06-17T18:12:45Z

Another option is to add it to account manager as a method.

joe-p · 2024-06-17T18:14:33Z

Another option is to add it to account manager as a method.

Yeah I like that. Makes it more clear that it's abstraction and not a transaction type

robdmoore · 2024-06-18T02:15:03Z

Cool, done.

Funnily enough it was how I originally wrote it, but then I decided to make it a composer thing so you could get the transaction separate to sending it. I think that's a minor benefit though.

joe-p · 2024-06-18T18:10:21Z

small nit: I would change rekeyAccount to rekeyAddress. In general as we start to get HD wallet support we should try to avoid using "account" to refer to a single address.

joe-p

@robdmoore can you remind me why we decided to support a union of string | TransactionSignerAccount for address fields? When would the TransactionSignerAccount be useful? I think it might be a bit confusing especially when I was looking at account.rekeyed. I suppose it's not clear when I'd want to use this vs setting the signer for address.

Also, I see a lot of changes to the formatting of how we saw "Algos". Personally I think non-plural ALGO is the most common. Ie. "I have 10 ALGO" vs "I have 10 Algos"

joe-p · 2024-07-04T14:35:19Z

docs/capabilities/account.md


+// Note: if a signing account is passed into `algorand.account.rekeyAccount` then you don't need to call `rekeyedAccount` to register the new signer
 const rekeyedAccount = algokit.rekeyedAccount(newAccount, account.addr)


Suggested change

const rekeyedAccount = algokit.rekeyedAccount(newAccount, account.addr)

const rekeyedAccount = Algorand.account.rekeyed(newAccount, account.addr)

joe-p · 2024-07-04T14:36:09Z

docs/capabilities/algorand-client.md

@@ -2,7 +2,7 @@

 `AlgorandClient` is a client class that brokers easy access to Algorand functionality. It's the [default entrypoint](../README.md#usage) into AlgoKit Utils functionality.

-The main entrypoint to the bulk of the functionality in AlgoKit Utils is the `AlgorandClient` class, most of the time you can get started by typing `algokit.AlgorandClient.` and choosing one of the static initialisation methods to create an [Algorand client](./capabilities/algorand-client.md), e.g.:
+The main entrypoint to the bulk of the functionality in AlgoKit Utils is the `AlgorandClient` class, most of the time you can get started by typing `algokit.AlgorandClient.` and choosing one of the static initialisation methods to create an [Algorand client](../code/classes/types_algorand_client.AlgorandClient.md), e.g.:


Aren't we now recommending to import AlgorandClient directly?

Yep this needs to be fixed

joe-p · 2024-07-04T14:45:11Z

src/indexer-lookup.spec.ts

    const secondAccount = await generateAccount({
-      initialFunds: algokit.algos(1),
+      initialFunds: (1).algos(),
      suppressLog: true,
    })
    const createParams = await getTestingAppCreateParams(testAccount, {


This line can be deleted

joe-p · 2024-07-04T14:50:13Z

src/types/algorand-client.ts

@@ -118,8 +135,13 @@ export class AlgorandClient {
    return this._accountManager
  }

+  /** Methods for interacting with assets. */
+  public get asset() {
+    return this._assetManager


Should the bulk functions go in the AssetManager?

robdmoore · 2024-07-04T16:44:56Z

why we decided to support a union of string | TransactionSignerAccount for address fields?

It was so that you could pass in the result of something like const alice = algorand.account.random() in i.e. you can pass in alice and not have to do alice.addr i.e. algorand.send.payment({from: alice, to: bob}) reads much nicer than algorand.send.payment({from: alice.addr, to: bob.addr})

We could remove that and just stick with string of course if we felt it was too confusing...

robdmoore · 2024-07-04T16:46:02Z

Also, I see a lot of changes to the formatting of how we saw "Algos". Personally I think non-plural ALGO is the most common. Ie. "I have 10 ALGO" vs "I have 10 Algos"

It's a tough one because I see it written in different ways everywhere, it's wildly inconsistent. I'm not opposed to just using ALGO though, more than happy with that?

robdmoore · 2024-07-04T16:46:56Z

Should the bulk functions go in the AssetManager?

I thought about that. Maybe that makes sense? I put in account because it was being done in the context of an account, but AccountManager is a really big class now and you could argue either way, so maybe AssetManager is better?

joe-p · 2024-07-04T17:00:29Z

It was so that you could pass in the result of something like const alice = algorand.account.random()

This begs the question... why not just have .random return a string? Returning an object with the signer makes sense when we are concerned about legacy interop, but now that we're deprecating legacy utils functions maybe it just makes more sense to return a string? I suppose the question really is where do we expect developers to use the signer function manually rather than using it implicitly through the AlgorandClient

It's a tough one because I see it written in different ways everywhere, it's wildly inconsistent. I'm not opposed to just using ALGO though, more than happy with that?

Yeah I don't have a strong opinion on it but consistency is nice at least within the libs we control. Perhaps we can just do a quick poll in slack to see what everyone prefers and just stick with that everywhere.

I thought about that. Maybe that makes sense? I put in account because it was being done in the context of an account, but AccountManager is a really big class now and you could argue either way, so maybe AssetManager is better?

Yeah I can see the argument either way. I personally leans towards AssetManager but have no objections for AccountManager. Although since AccountManager is already a big class and these are relatively uncommon functions to use, it might make more sense to put under AssetManager just for that reason.

robdmoore · 2024-07-04T17:10:23Z

I like the simplicity of returning a string, but it feels very wrong from an object oriented / code semantics perspective. ‘String’ is too generic a type for something that important and it would make it much harder to detect when you’re passing the wrong thing in the wrong place maybe?

agree on the other two. Did you want to raise the poll for algos?

joe-p · 2024-07-04T17:20:01Z

I like the simplicity of returning a string, but it feels very wrong from an object oriented / code semantics perspective. ‘String’ is too generic a type for something that important and it would make it much harder to detect when you’re passing the wrong thing in the wrong place maybe?

I agree but also think that's just the nature of nominal type systems. I'm not convinced it's worth creating an object just for this purpose.

agree on the other two. Did you want to raise the poll for algos?

Just posted in slack 👍

docs/capabilities/algorand-client.md

docs/capabilities/account.md

docs/capabilities/asset.md

src/types/algorand-client.ts

…of transaction modules * Collapsed the second object in algorand.send.type(params, *executeOptions*) to combine into params and make it easier to use * Added `algorand.client.getTestNetDispenserFromEnvironment` * Added `algorand.account.assetBulkOptIn` * Added `algorand.account.assetBulkOptOut` * Added `algorand.account.ensureFunded` * Added `algorand.account.ensureFundedFromEnvironment` * Added `algorand.account.ensureFundedFromTestNetDispenserApi` * Added `algorand.send/transaction.rekey` * Added `algorand.send/transaction.assetOptOut`

Added: * `AssetManager` class and `algorand.asset.getById()` * `indexer` export off of `@algorandfoundation/algokit-utils` as the future home of all indexer methods Changed: * Order of `algorand.account.rekeyed()` parameters to be `rekeyed(sender, signer)` since it conceptually makes more sense (the sender is rekeyed so should come first) * All microAlgo return values from `algorand.account.getInformation()` now return an `AlgoAmount`, renamed `amount` to `balance` and `round` to `validAsAtRound` (which is now a `bigint` for broader consistency) * Return value of `algorand.send.assetCreate` now includes `{ assetId: bigint }` Deprecated: * `algokit.createAsset` * `algokit.assetOptIn` * `algokit.assetOptOut` * `algokit.assetBulkOptIn` * `algokit.assetBulkOptOut` * `algokit.ensureFunded` * `algokit.transferAsset` * `algokit.rekeyAccount` * `algokit.transferAlgos`

…`AccountManager` Added: * `algorand.account.rekeyAccount()` Removed: * `algorand.send.rekey()`

…, asset, dispenser client, indexer, testing, transfer

…g to brand guidelines

Moved account.assetBulkOptIn -> asset.bulkOptIn Moved account.assetBulkOptOut -> asset.bulkOptOut Moved account.getAssetInformation -> asset.getAccountInformation Fixed other miscellaneous issues

robdmoore · 2024-08-21T14:03:14Z

Regardless of whether we decide to do string or not, it makes sense to decouple that from this PR since it's a much wider / bigger / unrelated change.

I also didn't change rekeyAccount to rekeyAddress because there are many other things that tie account = single address, that's a broader change that will somehow need to be incorporated into the Algorand ecosystem with thought to avoid confusion.

Otherwise incorporated all the other feedback.

…ual transactions refactor: Refactored AlgorandClient to have separate classes for `send` and `transaction` refactor: Refactored legacyTransactionBridge to not have a dependency of AlgorandClient

src/transaction/legacy-bridge.ts

src/transaction/transaction.ts

src/transfer/transfer-algos.ts

src/types/algorand-client-sender.ts

neilcampbell · 2024-08-28T03:55:31Z

src/types/algorand-client-sender.ts

+export type SendSingleTransactionResult = SendAtomicTransactionComposerResults & ConfirmedTransactionResult
+
+/** Orchestrates sending transactions for `AlgorandClient`. */
+export class AlgorandClientSender {


Should we adjust the name to be more consistent with the creator. In that case it would become AlgorandClientTransactionSender.

src/types/algorand-client-transaction-creator.ts

…t`, retained existing methods to avoid breaking changes

…onsistency

Breaking changes: * Renamed `AlgokitComposer` to `AlgoKitComposer` to match AlgoKit naming conventions * Collapsed the second object in algorand.send.type(params, *executeOptions*) to combine into params and make it easier to use based on devrel feedback * Order of `algorand.account.rekeyed()` parameters to be `rekeyed(sender, signer)` since it conceptually makes more sense (the sender is rekeyed so should come first) * All microAlgo return values from `algorand.account.getInformation()` now return an `AlgoAmount`, renamed `amount` to `balance` and `round` to `validAsAtRound` (which is now a `bigint` for broader consistency) * Renamed `algorand.account.getAssetInformation` to `algorand.asset.getAccountInformation` Added: * Return value of `algorand.send.assetCreate` now includes `{ assetId: bigint }` * Up to date documentation for rekeyAccount, AlgorandClient, asset, dispenser client, indexer, testing, transfer * Up to date documentation for how to refer to an Algo amount (Algo (not plural) in general and ALGO (not plural) when referring to a specific amount) * `AssetManager` class and `algorand.asset.getById()`, `algorand.asset.bulkOptIn`, and `algorand.asset.bulkOptOut` * `indexer` export off of `@algorandfoundation/algokit-utils` as the future home of all indexer methods * Added `algorand.client.getTestNetDispenserFromEnvironment` * Added `algorand.account.assetBulkOptIn` * Added `algorand.account.assetBulkOptOut` * Added `algorand.account.ensureFunded` * Added `algorand.account.ensureFundedFromEnvironment` * Added `algorand.account.ensureFundedFromTestNetDispenserApi` * Added `algorand.account.rekeyAccount` * Added `algorand.send/transaction.assetOptOut` * Added `buildTransactions` method to `AlgoKitComposer` so you can build transactions without needing to register a signer * Added `.algo` and `.microAlgo` methods/properties in place of the plural version for `AlgoAmount` to reflect the current guidance on how to represent Algo amounts, kept previous versions of those methods for now to avoid the breaking change Deprecated the following in favour of AlgorandClient functionality: * `algokit.createAsset` * `algokit.assetOptIn` * `algokit.assetOptOut` * `algokit.assetBulkOptIn` * `algokit.assetBulkOptOut` * `algokit.ensureFunded` * `algokit.transferAsset` * `algokit.rekeyAccount` * `algokit.transferAlgos` BREAKING CHANGE: Numerous --------- Co-authored-by: Neil Campbell <neil.campbell@makerx.com.au>

robdmoore requested review from joe-p and neilcampbell June 17, 2024 17:24

robdmoore commented Jun 17, 2024

View reviewed changes

docs/capabilities/algorand-client.md Show resolved Hide resolved

robdmoore commented Jun 17, 2024

View reviewed changes

robdmoore mentioned this pull request Jun 18, 2024

docs: AlgorandClient #286

Closed

joe-p reviewed Jul 4, 2024

View reviewed changes

neilcampbell approved these changes Jul 9, 2024

View reviewed changes

robdmoore added 3 commits August 21, 2024 16:40

docs: Started writing docs for algorand client (WIP)

485c5e2

robdmoore added 4 commits August 21, 2024 16:43

feat: Moved from rekey being a transaction type to being a method on …

e248c65

…`AccountManager` Added: * `algorand.account.rekeyAccount()` Removed: * `algorand.send.rekey()`

docs: Added up to date documentation for rekeyAccount, AlgorandClient…

3b9f9f7

…, asset, dispenser client, indexer, testing, transfer

docs: PR feedback - docs tweaks and consistent Algo/ALGO use accordin…

54b59fa

…g to brand guidelines

fix: Pull request feedback

2dad415

Moved account.assetBulkOptIn -> asset.bulkOptIn Moved account.assetBulkOptOut -> asset.bulkOptOut Moved account.getAssetInformation -> asset.getAccountInformation Fixed other miscellaneous issues

robdmoore force-pushed the deprecate-2 branch from d9fe41c to 2dad415 Compare August 21, 2024 14:01

fix: remove AlgorandClient memoize in AlgorandFixture

acde1a2

neilcampbell force-pushed the deprecate-2 branch from 10a069b to acde1a2 Compare August 21, 2024 15:46

robdmoore added 5 commits August 28, 2024 00:28

Removed the circular dependency in App Client

8b21fd7

Renamed AlgokitComposer to AlgoKitComposer

003934e

Fixing broken test

dfdd676

chore: Fixing vulnerable dependency

22d3817

fix: No longer require registration of a signer when building individ…

a5b03dd

…ual transactions refactor: Refactored AlgorandClient to have separate classes for `send` and `transaction` refactor: Refactored legacyTransactionBridge to not have a dependency of AlgorandClient

robdmoore force-pushed the deprecate-2 branch from 8a99bd5 to a5b03dd Compare August 27, 2024 17:40

fix: Removing circular dependency

c0313a0

robdmoore force-pushed the deprecate-2 branch from 4d0cd30 to c0313a0 Compare August 27, 2024 17:47

fix: Resolving circular dependency

f88b4c8

robdmoore force-pushed the deprecate-2 branch from 5e6d846 to f88b4c8 Compare August 27, 2024 18:03

neilcampbell reviewed Aug 28, 2024

View reviewed changes

robdmoore added 2 commits August 28, 2024 13:49

chore: PR review

6ed993e

fix: Switched to singular .algo and .microAlgo methods for `AlgoAmoun…

fed61c9

…t`, retained existing methods to avoid breaking changes

neilcampbell approved these changes Aug 28, 2024

View reviewed changes

Renamed AlgorandClientSender to AlgorandClientTransactionSender for c…

3c637f2

…onsistency

robdmoore force-pushed the deprecate-2 branch from 8bff754 to 3c637f2 Compare August 28, 2024 06:15

robdmoore merged commit 2aebcfd into main Aug 28, 2024
2 checks passed

robdmoore deleted the deprecate-2 branch August 28, 2024 10:55

robdmoore mentioned this pull request Sep 1, 2024

App deprecation #310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Deprecate transfer and asset modules #291

feat: Deprecate transfer and asset modules #291

robdmoore commented Jun 17, 2024 •

edited

Loading

robdmoore Jun 17, 2024

robdmoore Jun 17, 2024

robdmoore Jun 17, 2024

robdmoore Jun 17, 2024

robdmoore Jun 17, 2024

robdmoore Jun 17, 2024

joe-p Jul 4, 2024

robdmoore Jun 17, 2024

joe-p commented Jun 17, 2024

robdmoore commented Jun 17, 2024

robdmoore commented Jun 17, 2024 •

edited

Loading

joe-p commented Jun 17, 2024

robdmoore commented Jun 17, 2024

joe-p commented Jun 17, 2024

robdmoore commented Jun 18, 2024

joe-p commented Jun 18, 2024

joe-p left a comment

joe-p Jul 4, 2024

joe-p Jul 4, 2024

robdmoore Jul 4, 2024

joe-p Jul 4, 2024

joe-p Jul 4, 2024

robdmoore commented Jul 4, 2024

robdmoore commented Jul 4, 2024

robdmoore commented Jul 4, 2024

joe-p commented Jul 4, 2024

robdmoore commented Jul 4, 2024 •

edited

Loading

joe-p commented Jul 4, 2024

robdmoore commented Aug 21, 2024

neilcampbell Aug 28, 2024


		export async function generateTestAsset(client: Algodv2, sender: Account, total?: number) {
		export async function generateTestAsset(algorand: AlgorandClient, sender: string, total?: number) {

		@@ -0,0 +1,198 @@
		import { describe, test } from '@jest/globals'

		@@ -0,0 +1,378 @@
		import { describe, test } from '@jest/globals'


		// Note: if a signing account is passed into `algorand.account.rekeyAccount` then you don't need to call `rekeyedAccount` to register the new signer
		const rekeyedAccount = algokit.rekeyedAccount(newAccount, account.addr)

	const rekeyedAccount = algokit.rekeyedAccount(newAccount, account.addr)
	const rekeyedAccount = Algorand.account.rekeyed(newAccount, account.addr)

feat: Deprecate transfer and asset modules #291

feat: Deprecate transfer and asset modules #291

Conversation

robdmoore commented Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-p commented Jun 17, 2024

robdmoore commented Jun 17, 2024

robdmoore commented Jun 17, 2024 • edited Loading

joe-p commented Jun 17, 2024

robdmoore commented Jun 17, 2024

joe-p commented Jun 17, 2024

robdmoore commented Jun 18, 2024

joe-p commented Jun 18, 2024

joe-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robdmoore commented Jul 4, 2024

robdmoore commented Jul 4, 2024

robdmoore commented Jul 4, 2024

joe-p commented Jul 4, 2024

robdmoore commented Jul 4, 2024 • edited Loading

joe-p commented Jul 4, 2024

robdmoore commented Aug 21, 2024

Choose a reason for hiding this comment

robdmoore commented Jun 17, 2024 •

edited

Loading

robdmoore commented Jun 17, 2024 •

edited

Loading

robdmoore commented Jul 4, 2024 •

edited

Loading