Open barnjamin opened 1 year ago
https://github.com/algorand/js-algorand-sdk/blob/develop/src/composer.ts#L343 in fact we already have a check for TransactionWithSigner
Hi @algoanne @barnjamin
Would you allow me to make an attempt to this issue?
yes @shubhamshd, contributions are welcome here!
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 https://github.com/algorand/js-algorand-sdk/blob/4e1073d6a15b443a73dedf42b025173b397624b0/src/transaction.ts#L1376).
@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.
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.
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 https://github.com/algorand/js-algorand-sdk/issues/740#issuecomment-1452510712. 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.
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:
@algorandfoundation/algokit-utils
and algosdk
as dependencies. They are then free to import both packages in their code.@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
).@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.
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
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
Here is an easily reproducible live sandbox where you can observe the error yourself: CodeSandbox
Steps to replicate:
contract interactions demo
send transaction
(its a dummy ui so ignore the textfield)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
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 👍
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
.
@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 👍
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