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

feat: switch from soroban-react to generated libs #117

Merged
merged 42 commits into from
Jul 6, 2023

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented Jun 8, 2023

The Idea

Rather than use the @soroban-react suite of packages, this switches to packages generated by the new soroban contract bindings typescript command.

Get it Running

You should be able to run npm run install and npm run reset to get everything set up! Check out the new scripts in package.json to see the magic. Yes, it works with a postinstall script and generates the libraries directly into the node_modules folder. This smells bad but worked better than anything else we tried!

You'll also see that there's a cargo install_soroban step. This uses the install_soroban alias that's defined in the newly-added .cargo/config.toml file, and saves it to ./target/bin/soroban. If you want to work with your own local branch of soroban-cli, you can remove the built ./target/bin/soroban and add a symlink there instead pointing at your local CLI build's target folder.

@chadoh chadoh force-pushed the feat/generated-lib branch 3 times, most recently from c689821 to a32b966 Compare June 16, 2023 18:56
# paths = ["/path/to/override"] # path dependency overrides

[alias] # command aliases
install_soroban = "install --git https://github.com/ahalabs/soroban-tools --rev c00eca975046a3298bdb5b8d5d4dc8ebdcf4a128 --root ./target soroban-cli --debug"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can switch to stellar repo once stellar/stellar-cli#708 gets merged to main

@@ -6,7 +6,7 @@ on:
pull_request:

env:
RUSTFLAGS: -Dwarnings -Dclippy::all -Dclippy::pedantic
RUSTFLAGS: -Wclippy::all -Wclippy::pedantic
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 PR copied in the token example from https://github.com/stellar/soroban-examples. But the examples repo does not have the pedantic clippy linting, so this more rigid check failed. This makes the clippy check only warn (-W) rather than deny (-D).

@chadoh chadoh marked this pull request as ready for review June 16, 2023 20:42
@Shaptic
Copy link
Contributor

Shaptic commented Jun 16, 2023

I ran this with --root-dir . and it completely wiped my repo LOL, so I dunno if that's very good behavior given the innocuous where to place generated project help text

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

buncha small comments but otherwise lgtm 👍

package.json Outdated
"dev": "next dev",
"build": "next build",
"start": "next start",
"lint": "next lint"
"lint": "next lint",
"postinstall": "./target/bin/soroban contract bindings typescript --wasm ./target/wasm32-unknown-unknown/release/soroban_crowdfund_contract.wasm --id $(cat ./.soroban-example-dapp/crowdfund_id) --root-dir ./node_modules/crowdfund-contract --network futurenet --contract-name crowdfund-contract && ./target/bin/soroban contract bindings typescript --wasm ./target/wasm32-unknown-unknown/release/abundance_token.wasm --id $(cat ./.soroban-example-dapp/abundance_token_id) --root-dir ./node_modules/abundance-token --network futurenet --contract-name abundance-token"
Copy link
Contributor

Choose a reason for hiding this comment

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

a for loop on *.wasm could make this a little easier to grok but then you'd need to figure out the IDs somehow so maybe it'll actually be even more confusing hah

Copy link
Contributor Author

@chadoh chadoh Jun 19, 2023

Choose a reason for hiding this comment

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

We also considered putting it in the initialize.sh script. I like the postinstall because it guarantees that it runs after install, and install actually removes "incorrect" folders, which is essentially what these are, so you need to re-add them right away or you start getting errors.

I didn't want to use postinstall 😞

Other things we tried:

  • Generating them to target/js-clients/*, then adding them as dependencies with, for example, "crowdfund-contract": "file:./target/js-clients/crowdfund-contract". This worked most of the time, but failed on CI at the npm ci step. It seemed like maybe npm ci would cause NPM to fail to correctly resolve dependencies within the target/js-clients/* folders, but only in CI, not locally? You can see the failure here; it was very baffling.
  • Switching to yarn instead of npm. This required changing "file:./target/js-clients/*" to just "./target/js-clients/*", but at least it worked with yarn install --frozen-lockfile on CI.

We tried multiple variations of these, but we fretted that we would be inundated with support requests for whatever just-right permutation we landed on, since it all seemed so brittle.

Ultimately, we shook our heads at the messy state of local dependencies in the JS ecosystem, and we settled on the postinstall script. It's weird that this means dependencies aren't listed in dependencies, but if everything works then... ¯\_(ツ)_/¯ ???

Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the breakdown of the history behind this decision, heh, the knowledge-share should help with the "well why didn't you try x" I'm sure we'll see later after this is forgotten hah

Copy link
Contributor

Choose a reason for hiding this comment

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

✋ I was here and asked this question

package.json Outdated Show resolved Hide resolved
Comment on lines +13 to +16
const n = undivided.valueOf() < BigInt(Number.MAX_SAFE_INTEGER)
? Number(undivided) / (10 ** decimals)
: (undivided.valueOf() / (10n ** BigInt(decimals)));
return String(n);
Copy link
Contributor

Choose a reason for hiding this comment

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

you could do a scale-up by 1e7 then a shift of the decimal point 🤔

also shouldn't this check the result is under MAX_SAFE_INT rather than the value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the value is over MAX_SAFE_INT then you need to perform BigInt division. You can't mix BigInt and Number types during the division operation.

scale-up by 1e7 then a shift of the decimal point

Like, using string manipulation? Maybe. Feels a little dirty, but then again, so does what I've written here.

I don't think I nailed this logic, but it's been working well enough for the current app. I think maybe we merge as-is and then someone (you?) can send follow-up PRs with improvements. And maybe tests!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying! That makes sense. Yeah, I did mean using string manips, see this: stellar/js-stellar-base#607 (comment) 🧠 agreed that we can just improve this later; ideally this sorta stuff would have helpers in stellar-base aka soroban-client, anyway

shared/utils.tsx Show resolved Hide resolved
);

return xdr.ScVal.scvI128(new xdr.Int128Parts({lo, hi}));
export function scValToJs<T>(val: xdr.ScVal): T {
Copy link
Contributor

Choose a reason for hiding this comment

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

gonna use this as a trampoline for stellar/js-stellar-base#628, thanks!!

@paulbellamy
Copy link
Contributor

Seems like there are a ton of clippy errors in the abundance token, so running make fails.

@paulbellamy
Copy link
Contributor

This also doesn't work in standalone mode anymore (because the RPC_URL is hardcoded to futurenet). We should either fix that (which I'd prefer) or remove standalone mode (as a stopgap).

interface GeneratedLibrary {
Server: SorobanClient.Server
CONTRACT_ID_HEX: string
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat

Copy link
Contributor

@paulbellamy paulbellamy left a comment

Choose a reason for hiding this comment

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

Almost there. A couple failing build and standalone issues

Comment on lines +64 to +75
case xdr.ScValType.scvAddress(): {
return Address.fromScVal(val).toString() as unknown as T;
}
case xdr.ScValType.scvString(): {
return val.str().toString() as unknown as T;
}
case xdr.ScValType.scvSymbol(): {
return val.sym().toString() as unknown as T;
}
case xdr.ScValType.scvBytes(): {
return val.bytes() as unknown as T;
}
Copy link
Contributor

@Shaptic Shaptic Jun 22, 2023

Choose a reason for hiding this comment

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

realizing on a second look after doing some work to incorporate this into stellar-base@soroban, you can leverage .value() on the XDR structure if the effect is the same, e.g.:

        case xdr.ScValType.scvBool():
        case xdr.ScValType.scvU32():
        case xdr.ScValType.scvI32():
        case xdr.ScValType.scvString():
        case xdr.ScValType.scvBytes():
            return val.value() as unknown as T;

because if e.g. val.switch() == scvBytes, then val.value() == val.bytes()

chadoh and others added 16 commits July 5, 2023 14:02
- don't use `soroban lab token` to generate/deploy token
- instead, copy example token from examples repo
- remove `../examples` reference in `generate` script
- add `target/bin/soroban` with pinned version of `soroban` to be used
  throughout initialize.sh
This copies ideas from the contract in
https://github.com/AhaLabs/soroban-abundance-token, allowing anyone to
mint themselves tokens. Now this can be used from the frontend directly,
so we no longer need to embed the admin's secret key in the JS. Hooray!

Upon closer inspection, we didn't need any of those constants anymore,
since the contract IDs are embedded in the token libraries.
The whole page no longer reloads after a pledge. Instead, FormPledge now
takes an `onPledge` param, which it calls after the pledge succeeds.

The now-somewhat-awkward `resultSubmit` has also been filled back out to
match what the modal expects, so the modal looks correct.
The CI script needs the values there before it can generate the
libraries, of course.

And the `token_admin*` stuff isn't needed anymore.
Replace `run-script` with the much more common alias `run` to match the
rest of the workflow file
chadoh added 19 commits July 5, 2023 14:11
and add .cargo/config.toml to match CI settings
This gets the subscriptions working, and finally breaks all dependence
on the @soroban-react packages. The logic is a little confusing, but it
works!
This is a hook that can be copy/pasted into other React projects that
need such functionality, to work alongside their generated libraries
until these generated libraries expose subscription logic natively.
Uses the version with updated freighter-api stellar/stellar-cli#708
Since d771b3f, the default settings are
now set in `.cargo/config.toml`
Everything looks correct, but when running the app it fails with:

    Error: Cannot connect to insecure soroban-rpc server
@kalepail kalepail self-requested a review July 6, 2023 17:19
Copy link
Contributor

@kalepail kalepail left a comment

Choose a reason for hiding this comment

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

lgtm. If there's no reason not to I vote we go ahead and merge this. I love it!

@paulbellamy paulbellamy merged commit c2924f0 into stellar:main Jul 6, 2023
@chadoh chadoh deleted the feat/generated-lib branch July 6, 2023 18:10
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.

6 participants