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

Tweaks to AlgorandClient: #248

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

robdmoore
Copy link
Contributor

  • Rename AlgoKitClient to AlgorandClient and added static construction methods
  • Added fluent with* methods to AlgorandClient to allow for fluent configuration
  • Extracted client related stuff to ClientManager and account stuff to AccountManager and added extra functionality to both
  • Add transaction and confirmation to the result of sending a single transaction for better ergonomics
  • Added ability to specify suggested params and create a copy of suggested params when providing it
  • Moved classes to types directory
  • Added getAccountInformation to make tests terser (this method should have been added ages ago)
  • Incorporated client into testing fixture
  • Changed all possible bigint | numbers' to number (where it's impractical for them to hit 2^53)
  • Incorporated TransactionSignerAccount with TransactionSigner to allow for terser code
  • Rename ID to Id for consistency with rest of algokit utils

There is a txn dead error in the tests to do with valid round calculation that needs to be resolved and I haven't made any changes to composer yet, but there are some I want to suggest, this covers everything else though.

@robdmoore robdmoore requested a review from joe-p March 16, 2024 17:29
Comment on lines +102 to +118
const acc = await getTestAccount(
{ initialFunds: fixtureConfig?.testAccountFunding ?? algos(10), suppressLog: true },
transactionLoggerAlgod,
kmd,
)
algorandClient = AlgorandClient.fromClients({ algod: transactionLoggerAlgod, indexer, kmd }).withAccount(acc)
const testAccount = { ...acc, signer: algorandClient.account.getSigner(acc.addr) }
context = {
algod: transactionLoggerAlgod,
indexer: indexer,
kmd: kmd,
testAccount: await getTestAccount(
{ initialFunds: fixtureConfig?.testAccountFunding ?? algos(10), suppressLog: true },
transactionLoggerAlgod,
kmd,
),
generateAccount: (params: GetTestAccountParams) => getTestAccount(params, transactionLoggerAlgod, kmd),
testAccount,
generateAccount: async (params: GetTestAccountParams) => {
const account = await getTestAccount(params, transactionLoggerAlgod, kmd)
algorandClient.withAccount(account)
return { ...account, signer: algorandClient.account.getSigner(account.addr) }
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means generated accounts in testing will automatically be registered with the AlgorandClient that is now also available in the fixture.

* @param account The account to use.
* @returns A `TransactionSignerAccount` for the given account.
*/
private signerAccount<T extends SendTransactionFrom>(account: T): TransactionSignerAccount & { account: T } {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a private method that provides a bridge between the underlying AlgoKit account methods that return various subsets of SendTransactionFrom and the consistent TransactionSignerAccount type this class exposes. It still provides the underlying type via account just in case you want to do stuff with that (e.g. pull out sk etc.).

* Rename AlgoKitClient to AlgorandClient and added static construction methods
* Added fluent with* methods to AlgorandClient to allow for fluent configuration
* Extracted client related stuff to ClientManager and account stuff to AccountManager and added extra functionality to both
* Add transaction and confirmation to the result of sending a single transaction for better ergonomics
* Added ability to specify suggested params and create a copy of suggested params when providing it
* Moved classes to types directory
* Added getAccountInformation to make tests terser (this method should have been added ages ago)
* Incorporated client into testing fixture
* Changed all possible bigint | numbers' to number (where it's impractical for them to hit 2^53)
* Incorporated TransactionSignerAccount with TransactionSigner to allow for terser code
* Rename ID to Id for consistency with rest of algokit utils
@robdmoore robdmoore force-pushed the feat/client_and_composer-client-mods branch from c12ae03 to c4a7d20 Compare March 16, 2024 17:38
@joe-p
Copy link
Contributor

joe-p commented Mar 19, 2024

All looks good to me but ditching bigint does make me nervous. The SDK is going to all bigint for good reason imo. The more I think about I think having just bigint for all the numeric types makes the most sense (not even number | bigint). Most basic reasoning being that it's more representative of the actual data type. Also, while it's true that given how things currently work it's unlikely we'll need it, there could be changes in the future that would end up breaking deployed code (for example, supply cap increase or changes to how app/asset IDs are derived). There also could be forks of Algorand that would want to use this library.

Also having some places where bigint is used and some places where its not for the same underlying data type (uint64) can be a bit confusing

There is a txn dead error in the tests to do with valid round calculation that needs to be resolved

Presumably you fixed this? Tests seem to be passing for me.

@robdmoore
Copy link
Contributor Author

bigints are painful to work with, my vote is for using number for things we know will never be > 2^53, but happy to follow consensus from broader group of stakeholders.

The test error I get is URLTokenBaseHTTPError: Network request error. Received status 400 (Bad Request): TransactionPool.Remember: txn dead: round 418 outside of 397--407. As far as I can tell a transaction within a group is getting conflicting rounds.

@joe-p
Copy link
Contributor

joe-p commented Mar 19, 2024

bigints are painful to work with, my vote is for using number for things we know will never be > 2^53, but happy to follow consensus from broader group of stakeholders.

Consensus on devrel team was that it was better to use bigint everywhere. I agree they can be annoying, especially when working with numbers, but if everything is a bigint this should really be a problem. It also makes behavior much more predictable knowing most numbers should be bigints

The test error I get is URLTokenBaseHTTPError: Network request error. Received status 400 (Bad Request): TransactionPool.Remember: txn dead: round 418 outside of 397--407. As far as I can tell a transaction within a group is getting conflicting rounds.

Ah ok I was able to reproduce. Previously I was just running the client test by itself which was passing. When you run the full suite you get tests in parallel which affect the blocks. We just need to provide a wider window for testing.

@robdmoore
Copy link
Contributor Author

robdmoore commented Mar 20, 2024

Cool, can I make a suggestion that when receiving data we accept number | bigint, but when relaying information we provide bigint. I guess the problem may occur where we have a field that can be both though so if there are cases like that it may be better to just be consistent as annoying as bigint's are to deal with?

@joe-p
Copy link
Contributor

joe-p commented Mar 22, 2024

I made the change to just bigint. If it becomes a source of a lot of developer problems then we can re-evaluate and add a number union. I think it's easier to add a number union than to remove it.

I'll be merging this PR and recording bootcamp content with the current state of the base branch.

@joe-p joe-p merged commit d19267c into feat/client_and_composer Mar 22, 2024
joe-p added a commit that referenced this pull request Apr 17, 2024
* Tweaks to AlgorandClient:
* Rename AlgoKitClient to AlgorandClient and added static construction methods
* Added fluent with* methods to AlgorandClient to allow for fluent configuration
* Extracted client related stuff to ClientManager and account stuff to AccountManager and added extra functionality to both
* Add transaction and confirmation to the result of sending a single transaction for better ergonomics
* Added ability to specify suggested params and create a copy of suggested params when providing it
* Moved classes to types directory
* Added getAccountInformation to make tests terser (this method should have been added ages ago)
* Incorporated client into testing fixture
* Changed all possible bigint | numbers' to number (where it's impractical for them to hit 2^53)
* Incorporated TransactionSignerAccount with TransactionSigner to allow for terser code
* Rename ID to Id for consistency with rest of algokit utils

* use bigint where applicable

---------

Co-authored-by: Joe Polny <joepolny@gmail.com>
joe-p added a commit that referenced this pull request Apr 18, 2024
* feat: client and composer

* add composer and client to index.ts

* fix flatFee check

* fix typos

Co-authored-by: Tristan Menzel <tristan.menzel@makerx.com.au>

* implement isAbiValue

Co-authored-by: Tristan Menzel <tristanmenzel@gmail.com>

* txnMethodMap

* return txnWithSigners instead of mutating

co-authored-by: Tristan Menzel <tristanmenzel@gmail.com>

* improve methodCalls efficiency

co-authored-by: Tristan Menzel <tristanmenzel@gmail.com>

* refactor: Use switch blocks over if/else chains

* send and transaction

* rm debug from test

* implement max fee in composer

* start adding comments

* assetOptIn

* add more comments

* flatFee -> staticFee

* Uint8Array | string

* bigint and AlgoAmount

* Tweaks to AlgorandClient: (#248)

* Tweaks to AlgorandClient:
* Rename AlgoKitClient to AlgorandClient and added static construction methods
* Added fluent with* methods to AlgorandClient to allow for fluent configuration
* Extracted client related stuff to ClientManager and account stuff to AccountManager and added extra functionality to both
* Add transaction and confirmation to the result of sending a single transaction for better ergonomics
* Added ability to specify suggested params and create a copy of suggested params when providing it
* Moved classes to types directory
* Added getAccountInformation to make tests terser (this method should have been added ages ago)
* Incorporated client into testing fixture
* Changed all possible bigint | numbers' to number (where it's impractical for them to hit 2^53)
* Incorporated TransactionSignerAccount with TransactionSigner to allow for terser code
* Rename ID to Id for consistency with rest of algokit utils

* use bigint where applicable

---------

Co-authored-by: Joe Polny <joepolny@gmail.com>

* fix rollup issues

* to -> receiver

* getAccountAssetInformation

* change with... methods to set...

* persistent algorandClient

* defaultValidityWindow configuration

* fix AlgorandClient export

* closeRemainderTo

* onlineKeyReg

* update asset param comments

* AlgokitComposerParams

* AccountManager TSDoc

* generate docs

* refactor: Added final polish for beta release of AlgorandClient and AlgoKitComposer
test: Fixing indexer intermittent test timeouts with 20s waits for tests that wait for indexer

* rm aliases

* atc property and status check on build

* common build step in buildMethodCall

* export AlgorandClient

* fix typo

* use beta tag

* revert test changes made in 15ba017

* fix circular import

---------

Co-authored-by: Tristan Menzel <tristan.menzel@makerx.com.au>
Co-authored-by: Tristan Menzel <tristanmenzel@gmail.com>
Co-authored-by: Rob Moore (MakerX) <rob.moore@makerx.com.au>
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