Replace instanceof checks with other type check

[barnjamin]

Problem

when a different version of the algosdk is used from another dependency, the instanceof checks fail and result in confusing error messages.

Instead we should find another way to do the type check like https://www.typescriptlang.org/docs/handbook/2/typeof-types.html#the-typeof-type-operator

[barnjamin]

https://github.com/algorand/js-algorand-sdk/blob/develop/src/composer.ts#L343 in fact we already have a check for TransactionWithSigner

[shubhamshd]

Hi @algoanne @barnjamin
Would you allow me to make an attempt to this issue?

[algoanne]

yes @shubhamshd, contributions are welcome here!

[aorumbayev]

Hello @algoanne @jasonpaulos.

Wanted to supply some more detailed context for a similar edge case resulting in identical issue due to use of instanceof on custom types (such as

js-algorand-sdk/src/transaction.ts

Line 1376 in 4e1073d

return transactionLike instanceof Transaction

).

@joe-p initially reported the bug to us when playing with AlgoKit appclients generated by algokit-client-generator-ts on a React project that uses vite. AlgoKit TS AppClients are inspired by beaker-ts and in essence are just a self sufficient module that wraps some helpers for interacting with AlgoKit app client class coming from algokit-utils-ts. When issuing the call to abi methods via generated AppClient with abi parameters of type transaction, in short, algokit-utils-ts will process and load them up in atc which itself will call transaction.ts#L1376 on algosdk side. And it fails while performing instanceof check with address malformed error by incorrectly passing a valid Transaction object into Transaction constructor.

Why it fails on address malformed?

We think the cause of this behaviour is related to the vite using a different module loader in dev mode vs when you build the solution (in other words it only happens on vite in dev mode). In dev mode vite tries to flatten dependencies such that import * as x from '@algorandfoundation/algokit-utils' results in one bundle instead a collection of sub modules. Because algokit-utils is built as a commonjs module, optimiser seems to be then loading the browser version of algosdk instead of the esm version. Whilst imports of algosdk in user code are resolving to the esm version. Hence, during instanceof on Transaction (assembled in user code via esm algosdk version) vs Transaction (assembled in browser version of algosdk) it results in False because first is indeed an instance of a different class.

Suggestion on bug resolution

Before reporting this we have spend a good chunk of time trying to see if there are clean ways to introduce a workaround for this on AlgoKit but non of them are trivial and will potentially worsen developer experience around typescript typed clients. So as a result we think that its best to have this issue addressed on js-algorand-sdk, given that:
a) This bug was already reported even pre algokit typed clients and I assume it was happening on beaker-ts clients maintained by @barnjamin early this year. If you skim over Algorand discord you can also find mentions of address malformed from late last year, early this year.
b) js-algorand-sdk already partially resolving usages of instanceof in methods like #740 (comment). So its just a matter of adding similar methods for checks on objects like Transaction and etc. There is around 11 usages of instanceof across the js sdk codebase and only a subset of those need this further fix (cases for checks on Primitive types are completely valid and should probably stay as is, its only the custom types that are a concern)
c) Issue with malformed address isn't unique to algokit and can still happen under similar scenarios on any vite based frontend project that uses some package that relies on algosdk as a subdependency (while also having alsosdk as dependency at project root).
d) Issue with instanceof was reported on official vite repository a year ago. If you skim over it you'll notice that it also affected a lot of other repos in web2/web3 ecosystems. And in most of the cases people are coming up with solutions on their sdk sides to address and replace usage of instanceof. However, we think it still makes sense to address this on js algorand sdk regardless of whether vite patches that or not, vite is not the only bundler that can cause an edge case with 2 copies of algosdk being compared.

Lastly, this does not imply that users using vite are currently unable to use algokit ts generated clients we have 3 workarounds that support users on vite in such cases. We will add some temporary documentations on algokit ts generator repos for a note on usage with vite. Once this is resolved on js sdk we will remove it and bumping the sdk will essentially solve the issue for all vite (and potentially rollup.js) users.

[jasonpaulos]

Hi @aorumbayev, thank you for that information!

Before making a decision on instanceof usage in this library, can you help me understand what the current dependency situation is for users? I can imagine these scenarios, please let me know if one of these is accurate:

1. Users add @algorandfoundation/algokit-utils and algosdk as dependencies. They are then free to import both packages in their code.
2. Users add only @algorandfoundation/algokit-utils as a dependency. They can then import both @algorandfoundation/algokit-utils and algosdk (which is made available since it's a dependency of @algorandfoundation/algokit-utils).
3. Users add only @algorandfoundation/algokit-utils as a dependency. They then import only @algorandfoundation/algokit-utils in their code (no user imports of algosdk at all).

Asking because I'm first trying to understand how there can be two different algosdk packages in the first place. If there truly are two different versions of algosdk being used in the same process, removing the instanceof checks might only turn a fast failure turn into subtle unexpected behavior, which would not be desirable.

[barnjamin]

I can't recall the exact details but I know this also happened prior to algokit-utils existing, especially for packages that required algosdk but where the users of the package also required algosdk

A solution that seemed to work for packages that required algosdk was for them to make it a peer dependency, which eliminated the multi-version issue.

I think an improved type check could (?) help here since it'd look at the fields on the object which should (?) be version independent

[aorumbayev]

Hello @jasonpaulos and thanks for extra context @barnjamin ;)

To be specific the bug in @joe-p 's report occured for a vite based project with dependency scenario 1 (so it's option 1 in your follow up):

graph TB
  subgraph Scenario1
    B[User's vite based project]
    C[algorandfoundation/algokit-utils]
    D[algosdk]
    B --> C
    B --> D
  end

Loading

Here is an easily reproducible live sandbox where you can observe the error yourself: CodeSandbox

Steps to replicate:

1. Login to codesanbox after clicking link
2. Wait for npm install
3. Login via testnet wallet provider (for instance pera)
4. Open contract interactions demo
5. Click send transaction (its a dummy ui so ignore the textfield)
6. On a very first run it will prompt to deploy @joe-p 's Auction contract (i assume taken from beaker examples repo).
7. Right after deploy it will fail on address malformed

As I mentioned in the previous message the reason why we suspect that its highly likely caused by vite dev mode itself is due to the fact that if you then navigate to node_modules/algosdk and remove browser key from package.json and remove browser folder under dist and re run npm install within root of node_modules/algosdk this will force vite to use a cjs version of algosdk and the problem gets resolved.

I also tried option 2 from your message, you can also easily replicate that by removing project level algosdk and reinstalling via npm install. That way the problem still persists because vite will still grab the browser version algosdk that is build with webpack and address malformed still persists.

Additionally, as @barnjamin outlined, this was also the case for similar scenarios prior to algokit utils so I am pretty sure this can be reproduced on vite dev mode for any scenario 1,2 (where you have root project with algosdk + some package that uses algosdk as dependency OR some package that uses algosdk as dependency without user directly requiring algosdk at his project root)

---

As discussed during the call, using peerDependencies is not fixing the issue. We will have an item on backlog and turn algosdk into a peer dependency in a separate work thread regardless though

[aorumbayev]

Hey @jasonpaulos just checking on the current state for this item, from algokit side we can both bump the algosdk version and make it a peer dep within the same pr whenever this is addressed on the algosdk side. Feel free to ping us whenever there is a pr opened for this, can aid with running algosdk from a pr branch to confirm whether instanceof checks are no longer an issue 👍

[jasonpaulos]

To confirm, I believe the main problem is the instanceof checks used on the Transaction class. There are a few other places we use instanceof (against LogicSigAccount and ABI types), but since these have not been specifically mentioned, am I right to assume these are not causing issues?

If so, let me give some background on the Transaction use case. We have a variety of functions which manipulate transactions, and for backwards compatibility, these functions can operate on a Transaction class instance, or on a plain object with the correct properties.

We are actively working on a v3 release, and it's possible we could change this behavior in that release. Specifically, we could make it so that these functions would only accept Transaction instances, removing the need for them to use instanceof.

Would this be an acceptable solution? From a development perspective, it would be much preferred to remove the need for this check entirely instead of trying to retrofit the existing v2 code to perform the check without instanceof.

[aorumbayev]

@jasonpaulos thanks for the response, and I agree that sounds like it will solve the main edge case we hit with vite. Regardless though we have found a solution and fixed it on utils ts side so it's no longer an issue on the algokit side.

Presumably once your proposed solution is in place any similar edge cases on non algokit projects with similar setup are going to be fixed 👍