Skip to content
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

App client deprecation #311

Merged
merged 15 commits into from
Sep 19, 2024
Merged

App client deprecation #311

merged 15 commits into from
Sep 19, 2024

Conversation

robdmoore
Copy link
Contributor

@robdmoore robdmoore commented Sep 5, 2024

AlgorandClient deprecation pass of app-client, transaction and debugging modules (last ones left!) and associated documentation changes. Also added a v7 migration guide.

Breaking changes:

  • Moved ExecuteParams type from /types/composer to /types/transaction
  • executeParams in params input to appDeployer.deploy collapsed into the params object itself (not a breaking change from v6 since this is a new feature from last pull request)
  • persistSourceMaps now takes appManager rather than client
  • microAlgos property in AlgoAmount instances now returns a bigint rather than a number

Deprecations

  • getAppClient -> algorand.client.getAppClientById, algorand.client.getAppClientByCreatorAndName or algorand.client.getAppFactory (for create or deploy)
  • getAppClientById -> algorand.client.getAppClientById or algorand.client.getAppFactory (for create or deploy)
  • getAppClientByCreatorAndName -> algorand.client.getAppClientByCreatorAndName or algorand.client.getAppFactory (for create or deploy)
  • encodeTransactionNote -> AlgoKitComposer.arc2Note for ARC-2 and let devs convert to string or Uint8Array themselves otherwise
  • getSenderAddress -> Use algorand.client to interact with accounts, and use .addr to get the address and/or move from using SendTransactionFrom to TransactionSignerAccount and use .addr instead.
  • getTransactionWithSigner -> Use AlgorandClient / AlgoKitComposer to construct transactions instead or construct an algosdk.TransactionWithSigner manually instead.
  • getSenderTransactionSigner -> Use TransactionSignerAccount instead of SendTransactionFrom or use algosdk.makeBasicAccountTransactionSigner / algosdk.makeLogicSigAccountTransactionSigner.
  • signTransaction -> Use AlgorandClient / AlgoKitComposer to sign transactions or use the relevant underlying account.signTxn / algosdk.signLogicSigTransactionObject / multiSigAccount.sign / TransactionSigner methods directly.
  • sendTransaction -> Use AlgorandClient / AlgoKitComposer to send transactions.
  • sendParams property in the input to sendAtomicTransactionComposer -> new executeParams property
  • performAtomicTransactionComposerDryrun (deprecated Algorand feature)
  • sendGroupOfTransactions -> Use AlgoKitComposer (algorand.newGroup()) or AtomicTransactionComposer to construct and send group transactions instead.
  • capTransactionFee -> Use AlgoKitComposer and the maxFee field in the transaction params instead.
  • controlFees -> Use AlgoKitComposer and the maxFee and staticFee fields in the transaction params instead.
  • getTransactionParams -> Use suggestedParams ? { ...suggestedParams } : await algod.getTransactionParams().do() instead (AlgoKitComposer takes care of this for you so not likely to be needed anymore)
  • getAtomicTransactionComposerTransactions -> Use atc.clone().buildGroup() instead.
  • ApplicationClient -> AppClient and AppFactory
  • algorand.client.getAppClientByCreatorAndName returns an AppClient rather than an ApplicationClient
  • algorand.client.getAppClientById returns an AppClient rather than an ApplicationClient

New features

  • Added appManager.getGlobalState
  • Added AlgorandClientInterface type to decouple taking a dependency on AlgorandClient with creation of a circular dependency loop
  • Added current draft ARC-56 types to /types/app-arc56 with the root type being Arc56Contract
  • Added getABIEncodedValue, getABIDecodedValue, and getTupleType methods to /types/app-arc56 that allow you to encode and decode ABI values that use ARC-56 type strings (may reference structs, which can be nested)
  • Added AppFactory and AppClient classes and added algorand.client.getAppFactory and algorand.client.getAppClientByNetwork methods (and changed algorand.client.getAppId and algorand.client.getAppClientByCreatorAndName now return AppClient)
  • Added arc32ToArc56 method to /types/app-spec to allow conversion from ARC-32 to ARC-56 for backwards compatibility
  • Added addTransaction to AlgoKitComposer so you can add a transaction (with optional signer) to the transaction group
  • Added simulate to AlgoKitComposer so you can simulate a transaction group
  • Added AlgoKitComposer.arc2Note method to get an ARC-2 transaction note

BREAKING CHANGE: `ExecuteParams` has been moved from `types/composer` to `types/transaction`
@robdmoore robdmoore force-pushed the app-client-deprecation branch from 5a789eb to ebbf51a Compare September 5, 2024 11:06
…create and deploy apps and create app clients

test: Added test coverage of AppClient and AppFactory

BREAKING CHANGE: `persistSourceMaps` takes `appManager` rather than `client` now
@robdmoore robdmoore force-pushed the app-client-deprecation branch 2 times, most recently from f5ff4ce to c3683e0 Compare September 6, 2024 11:46
@joe-p
Copy link
Contributor

joe-p commented Sep 6, 2024

I find the deprecations a bit confusing in this PR. First, I'd propose we come up with a better naming differentiator than AppClient vs ApplicationClient, for example using ARC56Client.

I also don't think it makes sense to deprecate individual ApplicationClient methods. The class itself is being deprecated, so it just makes things much more confusing to have some of the methods marked as deprecated. For example

Use appClient.appId and appClient.appAddress from an AppClient instance instead.

Was rather confusing to me because it wasn't immediately obvious AppClient was a different class.

If someone is using a deprecated class, all of the class/instance methods are defacto deprecated so I think individual depcreation messages just make things more confusing.

feat: Added appClient.params.fundAppAccount and appClient.transactions.fundAppAccount
fix: Allow extraProgramPages to be passed into create and deploy call as override
@robdmoore robdmoore force-pushed the app-client-deprecation branch from c3683e0 to c8daa04 Compare September 6, 2024 12:11
@robdmoore
Copy link
Contributor Author

robdmoore commented Sep 6, 2024

It's rare that people will even need to interact with the class name itself since it's all exposed via AlgorandClient, but I'm not opposed to renaming it to Arc56Client if we think that's beneficial, but I don't think it's particularly necessary since ARC-56 is an implementation detail of the fact it's the AlgoKit app client.

The old classes and methods will be deleted after the next major version bump after this one so it's a temporary problem there are two classes with similar names and I was mostly thinking about what the end state of this will look like (also why it's in the same file rather than a different one because the end state that makes sense is to use that export path).

I agree it's unusual that there are per-method deprecations for a deprecated class, but they are purely there to provide migration hints for end users. Neil has been migrating Lora to AlgorandClient and commented that the deprecation notes had been extremely helpful with the migration process so I was trying to replicate that experience.

@robdmoore
Copy link
Contributor Author

(Noting I only added method deprecations to ApplicationClient where there are big differences from old to new)

docs/capabilities/app-client.md Outdated Show resolved Hide resolved
docs/capabilities/app-client.md Outdated Show resolved Hide resolved
docs/capabilities/app-client.md Outdated Show resolved Hide resolved
docs/capabilities/app-client.md Outdated Show resolved Hide resolved
docs/capabilities/app-client.md Outdated Show resolved Hide resolved
docs/v7-migration.md Outdated Show resolved Hide resolved
docs/v7-migration.md Outdated Show resolved Hide resolved
src/types/app-client.ts Show resolved Hide resolved
src/types/app-spec.ts Outdated Show resolved Hide resolved
src/types/app-client.ts Outdated Show resolved Hide resolved
@neilcampbell
Copy link
Contributor

neilcampbell commented Sep 6, 2024

I also don't think it makes sense to deprecate individual ApplicationClient methods. The class itself is being deprecated, so it just makes things much more confusing to have some of the methods marked as deprecated. For example

Marking a class as deprecated only tells the user that they need to switch to something else, it doesn't give enough granular detail about how they should do that and maintain the existing behaviour. Having methods marked as deprecated allows us to inform the user how to transition to the new code in a way that maintains the previous semantics and behaviour.

feat: Added a number of methods and types to ARC-56 to make interacting with it easier
src/types/app-arc56.ts Show resolved Hide resolved
src/types/app-arc56.ts Outdated Show resolved Hide resolved
src/types/app-arc56.ts Show resolved Hide resolved
src/types/app-arc56.ts Outdated Show resolved Hide resolved
@robdmoore robdmoore force-pushed the app-client-deprecation branch from ad50d43 to b95895f Compare September 13, 2024 17:36
@aorumbayev
Copy link
Contributor

@robdmoore minor comment but worth removing the zenhub workflows from gh workflows folder too as they aren't needed.

@aorumbayev
Copy link
Contributor

@robdmoore ci runs using node v18, but getting rollup errors on v22 when running locally:

➜ algokit-utils-ts (app-client-deprecation) ✔ npm run build      

> @algorandfoundation/[email protected] build
> run-s build:*


> @algorandfoundation/[email protected] build:0-clean
> rimraf dist coverage


> @algorandfoundation/[email protected] build:1-compile
> rollup -c --configPlugin typescript

loaded rollup.config.ts with warnings
(!) [plugin typescript] @rollup/plugin-typescript: Rollup 'sourcemap' option must be set to generate source maps.
[!] SyntaxError: Unexpected identifier 'assert'
    at compileSourceTextModule (node:internal/modules/esm/utils:337:16)
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:166:18)
    at callTranslator (node:internal/modules/esm/loader:436:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:442:30)
    at ModuleJob._link (node:internal/modules/esm/module_job:106:19)


ERROR: "build:1-compile" exited with 1.

Could be addressed as a minor enhancement outside of pr scope though but just wanted to note

@robdmoore
Copy link
Contributor Author

Thanks @aorumbayev! Looks like the assert error is coming from within the rollup typescript plugin. wessberg/rollup-plugin-ts#228

In the meantime, this library won't be able to be built on node.js 22

BREAKING CHANGE: microAlgo/s property in AlgoAmount now returns a bigint
@neilcampbell neilcampbell merged commit 0cd0743 into main Sep 19, 2024
2 checks passed
@neilcampbell neilcampbell deleted the app-client-deprecation branch September 19, 2024 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants