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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary use of v2 algod client #616

Merged
merged 3 commits into from
Jul 29, 2022
Merged

Remove unnecessary use of v2 algod client #616

merged 3 commits into from
Jul 29, 2022

Conversation

michaeldiamant
Copy link
Contributor

Extends #614 to remove what appears to be unnecessary v2 algod client usage. The initialization of a v2 client in an algod client led me to creating the PR.

Note - Predating the PR, there is a flaky test: Scenario: Getting transaction by ID. The PR does not attempt to remedy it.

@jasonpaulos
Copy link
Contributor

I don't have much context on this PR, but why would we remove v2 algod client usage in favor of v1 (which should be deprecated if it's not already)?

@michaeldiamant
Copy link
Contributor Author

@jasonpaulos Responding inline. If it doesn't address the question, let's speak live:

why would we remove v2 algod client usage in favor of v1

  • The test in question is a v1 API test. For reasons unclear to me (that's why I opened the PR), v2 algod client usage is intermingled. Like I mention in the description, it's strange to see added v2 initialization.
  • If I had to guess, I think v2 client was introduced for convenience of marshalling suggested params.
  • When we finally remove v1 API support, I expect we'll make test changes to use v2 client. But, I think it's out of scope for the dev mode network testing effort.

@michaeldiamant
Copy link
Contributor Author

For reviewers: The build failure associated with 3a8cbb1 should be fixed in #617.

Let's bring #617 in and then the build ought to pass here.

@michaeldiamant michaeldiamant marked this pull request as ready for review July 29, 2022 19:33
algochoi
algochoi approved these changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

LGTM

@michaeldiamant michaeldiamant merged commit 14f3a82 into dev-mode-testing Jul 29, 2022
@michaeldiamant michaeldiamant deleted the dev-mode-testing-patch branch July 29, 2022 19:49
algochoi added a commit that referenced this pull request Aug 1, 2022
@algochoi @michaeldiamant
Testing: Use Dev mode network for cucumber tests (#614)
e16cef3 
* Send zero txn to itself

* Refactor block advance functions

* Revise steps to allow for rekeying transient accounts

* Try to reduce flaky tests

* Move constant into step

* Add artificial sleep instead of sending blank txns when mimicking wait for block API

* Reduce flaky tests

* Remove unnecessary use of v2 algod client (#616)

* Remove unnecessary use of v2 algod client
* Add missing await for async function calls (#617)

* Rename rekey tag in makefile

* Revert testing branch back to master

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>
algochoi added a commit that referenced this pull request Aug 8, 2022
@algochoi @joe-p
@AlgoDoggo @bmdelacruz @algolucky @algojack @Eric-Warehime @barnjamin @fionnachan @jdtzmn @michaeldiamant
Merge develop branch into feature/box-storage (#620)
5353441 
* properly set trace maxWidth (#593)

* fix: safe intDecoding (#599)

* fix: safe intDecoding

* test: parse number in edge case

* refactor: remove optional chaining for node12

* Remove code that relies on node's path module (#598)

* Remove code that relies on node's path module

* Replace url-parse with built in WHATWG URL API

* Removed path-browserify fallback from webpack config

* Removed path-browserify and url-parse from npm dependencies

* Removed references to `path-browserify` in FAQ.md

* bump version

* Github-Actions: Adding PR title and label checks (#600)

* Remove unused/unmaintained templates (#607)

* Dev Tools: Source map decoder (#590)

* adding source map decoder

* Enhancement: Upgrade typedoc and plugins (#605)

* Update ts-node, typescript, and typedoc to latest

* docs: tealSign (#610)

* bump version and add to changelog

* update README.md for new version

* API: Support attaching signatures to standard and multisig transactions (#595)

* Add attach signature method to transaction class

* Add multisig external signature methods

* Fix failing multisig test

* Add signature length checks

* Add method to create an unsigned multisig transaction blob

* Rename multisig create methods and use unencoded transaction

* Refactor `createMultisigTransactionWithSignature` to use `createMultisigTransaction` method

* Fix algosdk createMultisigTransaction export

* Use MultisigMetadata without pks in new create method

* These types should be consolidated in the future, and addrs seems like a better convention to use long-term

* More descriptive test suite name

* AVM:  Consolidate TEAL and AVM versions (#609)

* Testing: Use Dev mode network for cucumber tests (#614)

* Send zero txn to itself

* Refactor block advance functions

* Revise steps to allow for rekeying transient accounts

* Try to reduce flaky tests

* Move constant into step

* Add artificial sleep instead of sending blank txns when mimicking wait for block API

* Reduce flaky tests

* Remove unnecessary use of v2 algod client (#616)

* Remove unnecessary use of v2 algod client
* Add missing await for async function calls (#617)

* Rename rekey tag in makefile

* Revert testing branch back to master

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

* Revert package lock from develop

Co-authored-by: Joe Polny <50534337+joe-p@users.noreply.github.com>
Co-authored-by: AlgoDoggo <93348148+AlgoDoggo@users.noreply.github.com>
Co-authored-by: Bryan Dela Cruz <bdshi13@outlook.com>
Co-authored-by: Lucky Baar <lucky.baar@algorand.com>
Co-authored-by: Jack <87339414+algojack@users.noreply.github.com>
Co-authored-by: Eric Warehime <eric.warehime@gmail.com>
Co-authored-by: Ben Guidarelli <ben.guidarelli@gmail.com>
Co-authored-by: Fionna Chan <fionnacst@gmail.com>
Co-authored-by: Jack Smith <jack.smith@algorand.com>
Co-authored-by: Jacob Daitzman <jdtzmn@gmail.com>
Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>
ahangsu added a commit that referenced this pull request Sep 6, 2022
@algochoi @joe-p
@AlgoDoggo @bmdelacruz @algolucky @algojack @Eric-Warehime @barnjamin @fionnachan @jdtzmn @michaeldiamant @ahangsu
Box support for Indexer endpoints (#619)
af80784 
* Add boxes endpoint with path tests

* Add lookup box by name api for indexer

* Merge develop branch into feature/box-storage (#620)

* properly set trace maxWidth (#593)

* fix: safe intDecoding (#599)

* fix: safe intDecoding

* test: parse number in edge case

* refactor: remove optional chaining for node12

* Remove code that relies on node's path module (#598)

* Remove code that relies on node's path module

* Replace url-parse with built in WHATWG URL API

* Removed path-browserify fallback from webpack config

* Removed path-browserify and url-parse from npm dependencies

* Removed references to `path-browserify` in FAQ.md

* bump version

* Github-Actions: Adding PR title and label checks (#600)

* Remove unused/unmaintained templates (#607)

* Dev Tools: Source map decoder (#590)

* adding source map decoder

* Enhancement: Upgrade typedoc and plugins (#605)

* Update ts-node, typescript, and typedoc to latest

* docs: tealSign (#610)

* bump version and add to changelog

* update README.md for new version

* API: Support attaching signatures to standard and multisig transactions (#595)

* Add attach signature method to transaction class

* Add multisig external signature methods

* Fix failing multisig test

* Add signature length checks

* Add method to create an unsigned multisig transaction blob

* Rename multisig create methods and use unencoded transaction

* Refactor `createMultisigTransactionWithSignature` to use `createMultisigTransaction` method

* Fix algosdk createMultisigTransaction export

* Use MultisigMetadata without pks in new create method

* These types should be consolidated in the future, and addrs seems like a better convention to use long-term

* More descriptive test suite name

* AVM:  Consolidate TEAL and AVM versions (#609)

* Testing: Use Dev mode network for cucumber tests (#614)

* Send zero txn to itself

* Refactor block advance functions

* Revise steps to allow for rekeying transient accounts

* Try to reduce flaky tests

* Move constant into step

* Add artificial sleep instead of sending blank txns when mimicking wait for block API

* Reduce flaky tests

* Remove unnecessary use of v2 algod client (#616)

* Remove unnecessary use of v2 algod client
* Add missing await for async function calls (#617)

* Rename rekey tag in makefile

* Revert testing branch back to master

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

* Revert package lock from develop

Co-authored-by: Joe Polny <50534337+joe-p@users.noreply.github.com>
Co-authored-by: AlgoDoggo <93348148+AlgoDoggo@users.noreply.github.com>
Co-authored-by: Bryan Dela Cruz <bdshi13@outlook.com>
Co-authored-by: Lucky Baar <lucky.baar@algorand.com>
Co-authored-by: Jack <87339414+algojack@users.noreply.github.com>
Co-authored-by: Eric Warehime <eric.warehime@gmail.com>
Co-authored-by: Ben Guidarelli <ben.guidarelli@gmail.com>
Co-authored-by: Fionna Chan <fionnacst@gmail.com>
Co-authored-by: Jack Smith <jack.smith@algorand.com>
Co-authored-by: Jacob Daitzman <jdtzmn@gmail.com>
Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

* Revert package lock

* Fix comments in box related APIs

* Add next token to indexer box API test

* rm unused step

* Update src/client/v2/algod/getApplicationBoxByName.ts

* Update .test-env

Co-authored-by: Joe Polny <50534337+joe-p@users.noreply.github.com>
Co-authored-by: AlgoDoggo <93348148+AlgoDoggo@users.noreply.github.com>
Co-authored-by: Bryan Dela Cruz <bdshi13@outlook.com>
Co-authored-by: Lucky Baar <lucky.baar@algorand.com>
Co-authored-by: Jack <87339414+algojack@users.noreply.github.com>
Co-authored-by: Eric Warehime <eric.warehime@gmail.com>
Co-authored-by: Ben Guidarelli <ben.guidarelli@gmail.com>
Co-authored-by: Fionna Chan <fionnacst@gmail.com>
Co-authored-by: Jack Smith <jack.smith@algorand.com>
Co-authored-by: Jacob Daitzman <jdtzmn@gmail.com>
Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>
Co-authored-by: Hang Su <hang.su@algorand.com>
Co-authored-by: Hang Su <87964331+ahangsu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@algochoi algochoi algochoi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michaeldiamant @jasonpaulos @algochoi