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

use non buffer format of fromXDR #32

Merged
merged 2 commits into from
May 31, 2023

Conversation

sreuland
Copy link
Collaborator

fixing an issue seen in downstream client usage in browser deployments with error on Buffer.from() of TypeError: this._buffer.readBigInt64BE is not a function when passing it to soroban client fromXDR() methods, if the downstream client app hasn't poly-filled 'Buffer' to a version that has readBigInt64BE() included, then seeing this error raised, it can be avoided by not using Buffer.from() and instead passing xdr encoded string directly to soroban client using fromXDR(xdr, 'base64')

@sreuland sreuland marked this pull request as draft May 31, 2023 00:28
@sreuland
Copy link
Collaborator Author

@esteblock , I first noticed this while running the dapp - stellar/soroban-example-dapp#105 (comment)

I think this change will resolve the issue, I'm trying to test this locally from dapp to confirm, do you by chance know how to change package.json from a client project such as dapp to point at a local directory for one of the soroban-react workspace packages? I'm trying

"@soroban-react/contracts": "../soroban-react/packages/contracts"

but not working at runtime, any insights on how to facilitate that type of local testing?

@Shaptic
Copy link
Collaborator

Shaptic commented May 31, 2023

if the downstream client app hasn't poly-filled 'Buffer' to a version that has readBigInt64BE() included

I might be missing some context here, but why doesn't the downstream client just... polyfill correctly?

@Shaptic
Copy link
Collaborator

Shaptic commented May 31, 2023

@sreuland for local testing, this should work: https://stackoverflow.com/a/14387210?

@esteblock
Copy link
Member

@sreuland @Shaptic will check all this tomorrow

@sreuland
Copy link
Collaborator Author

@Shaptic, @esteblock , this could also be fixed by requiring downstream app poly-fill on Buffer to a min version having readBigInt64BE, but that seemed a bit heavy to impose on users of this package, I initially took that approach from the dapp side also while debugging, had the next/webpack config changed to polyfill buffer and it does work, but taking a step back, realized the simplicity of just avoiding it altogether here instead by using the overloaded methods provided by soroban client sdk. let me know, I can bring that back into dapp, but I think then this pr needs to change to update the Readme to document the requirements on buffer polyfill.

@esteblock
Copy link
Member

esteblock commented May 31, 2023

@esteblock , I first noticed this while running the dapp - stellar/soroban-example-dapp#105 (comment)

I think this change will resolve the issue, I'm trying to test this locally from dapp to confirm, do you by chance know how to change package.json from a client project such as dapp to point at a local directory for one of the soroban-react workspace packages? I'm trying

"@soroban-react/contracts": "../soroban-react/packages/contracts"

but not working at runtime, any insights on how to facilitate that type of local testing?

So this is something I need to work on. Because what I usually would do is:

cd soroban-react/packages/contracts
yarn link
....
cd soroban-example-dapp
yarn remove @soroban-react/contracts
yarn link @soroban-react/contracts
yarn
yarn dev

Following here: https://classic.yarnpkg.com/en/docs/cli/unlink#yarn-unlink-package

But doing this I get the following error:

../soroban-react/packages/contracts/dist/useContractValue.js:39:0
Module not found: Can't resolve 'react'
  37 | Object.defineProperty(exports, "__esModule", { value: true });
  38 | exports.useContractValue = void 0;
> 39 | const react_1 = __importDefault(require("react"));
  40 | const SorobanClient = __importStar(require("soroban-client"));
  41 | let xdr = SorobanClient.xdr;
  42 | // Dummy source account for simulation. The public key for this is all 0-bytes.

Import trace for requested module:
../soroban-react/packages/contracts/dist/index.js
./components/organisms/pledge/index.tsx
./components/organisms/index.tsx
./pages/index.tsx

https://nextjs.org/docs/messages/module-not-found

I think is due to the react peer dependency of the library

There is a framework to test react libraries, called Jest. I have an open issue to add tests to this library #10

@sreuland
Copy link
Collaborator Author

sreuland commented May 31, 2023

@esteblock , yes that is same error with react dependency I'm seeing at browser runtime when trying to reference the workspace packages as local paths from dapp, the node_modules layout looks a bit different between the local path vs. npm repo, just the dist folder is there for the packages. I tried adding "react": "^18.2.0" to dependencies locally in soroban-react packages to see if that had an effect but still saw same issue.

@sreuland sreuland marked this pull request as ready for review May 31, 2023 18:42
packages/events/README.md Outdated Show resolved Hide resolved
@sreuland
Copy link
Collaborator Author

Hello @esteblock , I think we are have agreement on the approach and ready to merge. Do you mind releasing new minor rev's of the packages? Thanks!

@esteblock
Copy link
Member

Hello @esteblock , I think we are have agreement on the approach and ready to merge. Do you mind releasing new minor rev's of the packages? Thanks!

Yes, sure! Also, I will be very happy to add you as collaborators of the repo, what do you think?

@sreuland
Copy link
Collaborator Author

yes, sounds good as changes happening quite often in the stack, thanks!

@esteblock
Copy link
Member

Changes:

  • @soroban-react/connect-button: 5.0.1-alpha.0 => 5.0.2-alpha.0
  • @soroban-react/contracts: 5.0.1-alpha.0 => 5.0.2-alpha.0
  • @soroban-react/core: 5.0.1-alpha.0 => 5.0.2-alpha.0
  • @soroban-react/events: 5.0.1-alpha.0 => 5.0.2-alpha.0
  • @soroban-react/wallet-data: 5.0.1-alpha.0 => 5.0.2-alpha.0

@esteblock esteblock merged commit 23582c5 into paltalabs:main May 31, 2023
@esteblock
Copy link
Member

To give you access to the npm registry ( in order for you to publish, I will need your email addresses asociated to your npm account) Reach me on Discord. You will find me as esteblock in the stellar channel

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.

3 participants