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

V7 tweaks #317

Merged
merged 12 commits into from
Sep 29, 2024
Merged

V7 tweaks #317

merged 12 commits into from
Sep 29, 2024

Conversation

robdmoore
Copy link
Contributor

Tweaks based on feedback from various people including @neilcampbell and @SilentRhetoric to prepare for v7 finalisation

@@ -108,9 +108,9 @@ The create method is a wrapper over the `appCreate` (bare calls) and `appCreateM

```typescript
// Use no-argument bare-call
const { result, app } = factory.create()
const { result, appClient } = factory.send.bare.create()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SilentRhetoric even though it adds a little bit of verbosity we made the decision that every time a transaction is sent to the network you will see the word send (apart from the deploy call, but that's a special case since it may or may not send transactions and is a higher-order function).

As part of this we renamed execute to send (with a backwards compatible @deprecate) in AlgoKitComposer and renamed ExecuteParams to SendParams.

// Specify parameters for bare-call and override other parameters
const { result, app } = factory.create({
const { result, appClient } = factory.send.bare.create({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We also settled on appClient over app to meet your principle of clear, explicit naming @SilentRhetoric

method: 'create_application',
args: [1, 'something'],
})
```

If you want to construct a custom create call, use the underlying [`algorand.send.appCreate` / `algorand.transactions.appCreate` / `algorand.send.appCreateMethodCall` / `algorand.transactions.appCreateMethodCall` methods](./app.md#creation) then you can get params objects:
If you want to construct a custom create call, use the underlying [`algorand.send.appCreate` / `algorand.createTransaction.appCreate` / `algorand.send.appCreateMethodCall` / `algorand.createTransaction.appCreateMethodCall` methods](./app.md#creation) then you can get params objects:
Copy link
Contributor Author

@robdmoore robdmoore Sep 27, 2024

Choose a reason for hiding this comment

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

@SilentRhetoric transactions -> createTransaction to get the verb plus the consistency of singular rather than plural.

We didn't rename params because the correct name would be getTransactionParams and it's simply too verbose and the places you are most likely to use params are inline when nesting ABI method arguments where you want less verbosity. As such we updated the jsdoc comment to make it more obvious how it's used and feel it's one that people will follow examples to understand how to use (and honestly, it's a slightly more advanced use case anyway so I think it's OK if it's a bit less discoverable).

@@ -92,7 +92,7 @@ export function algorandFixture(fixtureConfig?: AlgorandFixtureConfig, config?:
const transactionLoggerAlgod = transactionLogger.capture(algod)
algorand = AlgorandClient.fromClients({ algod: transactionLoggerAlgod, indexer, kmd })
const acc = await getTestAccount({ initialFunds: fixtureConfig?.testAccountFunding ?? algos(10), suppressLog: true }, algorand)
algorand.setSignerFromAccount(acc).setSuggestedParamsTimeout(0)
algorand.setSignerFromAccount(acc).setSuggestedParamsCacheTimeout(0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SilentRhetoric while reviewing the method names we realised this method name was more descriptive as CacheTimeout

* @param algod An algod client
* @returns An object with transaction IDs, transactions, group transaction ID (`groupTransactionId`) if more than 1 transaction sent, and (if `skipWaiting` is `false` or unset) confirmation (`confirmation`)
*/
export const sendAtomicTransactionComposer = async function (atcSend: AtomicTransactionComposerToSend, algod: Algodv2) {
const { atc: givenAtc, sendParams, executeParams } = atcSend
const { atc: givenAtc, sendParams, ...executeParams } = atcSend
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of renaming to SendParams from ExecuteParams we also collapsed the values into the params object for sendAtomicTransactionComposer, which meets our new single param object style. This is a bit of a behind the scenes method anyway that's unlikely to be used broadly outside of utils itself and adding executeParams is a new thing in v7 beta anyway so this isn't a breaking change from v6.

type: algosdk.ABIReturnType
}
>
override readonly args: Array<Arc56MethodArg>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In various places we removed inline use of Expand<> since we found it caused some confusion when consuming from npm. Instead we have dedicated exported types that use Expand.

* @param params The parameters to create the app
* @returns The app client and the result of the creation transaction
*/
public async create(params?: AppFactoryCreateParams) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than a create method that takes bare or abi we split in 2 for consistency with app client. Makes the method less confusing to consume too.

@robdmoore robdmoore force-pushed the v7-tweaks branch 2 times, most recently from 64ebd04 to 7b59d1b Compare September 28, 2024 09:05
…ous and consistent naming

feat: Added transaction creation to app factory
refactor: Rename `execute` on `AlgoKitComposer` to `send` for consistency

BREAKING CHANGES: Various since initial v7 beta
* ExecuteParams -> SendParams
* sendAtomicTransactionComposer takes a params object that extends SendParams now
* algorand.transactions. is now algorand.createTransaction.
* appClient.transactions. is now appClient.createTransaction.
* appFactory.create/deploy now returns the app client in an appClient property rather than an app property
* appFactory.create is now appFactory.send.create and appFactory.send.bare.create
…sults in a sub-par experience in the npm consumed package
@robdmoore robdmoore force-pushed the v7-tweaks branch 2 times, most recently from 9c0cde6 to 8d59286 Compare September 29, 2024 15:05
@robdmoore robdmoore merged commit 4162e30 into main Sep 29, 2024
2 checks passed
@robdmoore robdmoore deleted the v7-tweaks branch September 29, 2024 15:25
robdmoore added a commit that referenced this pull request Sep 29, 2024
feat: Renamed app client and algorand client properties for more obvious and consistent naming
feat: Make the compile method public in app factory and app client
feat: Improving error stack trace propagation from sendAtomicTransactionComposer
feat: Added `setSigners` on `AccountManager` so you can copy signers from one `AccountManager` to another
feat: Added transaction creation to app factory
refactor: Rename `execute` on `AlgoKitComposer` to `send` for consistency
refactor: Removing use of Expand<> inline in method calls since it results in a sub-par experience in the npm consumed package
feat: Updated to latest ARC-56 spec
docs: Updated docs for v7

BREAKING CHANGES: Various since initial v7 beta
* ExecuteParams -> SendParams
* sendAtomicTransactionComposer takes a params object that extends SendParams now
* algorand.transactions. is now algorand.createTransaction.
* appClient.transactions. is now appClient.createTransaction.
* appFactory.create/deploy now returns the app client in an appClient property rather than an app property
* appFactory.create is now appFactory.send.create and appFactory.send.bare.create
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.

2 participants