Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@expenses
Copy link
Contributor

@expenses expenses commented Dec 13, 2019

There is a lot of shared logic between the test substrate/polkadot in-browser light clients I've been working on, so I'm putting it in this browser-utils crate.

For clarity: I'm still working out how to best approach certain changes needed to get the light clients to work properly, so I'm merging things piece-by-piece.

@expenses expenses requested a review from gnunicorn December 13, 2019 13:43
@gavofyork gavofyork added the A0-please_review Pull request needs code review. label Dec 13, 2019
@expenses expenses requested a review from amaury1093 December 15, 2019 10:54
@expenses expenses force-pushed the ashley-browser-utils branch from 222ce99 to e73aac3 Compare December 15, 2019 11:09
Copy link
Contributor

@gnunicorn gnunicorn left a comment

Choose a reason for hiding this comment

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

Had some minor things, aside from that I don't have any complaints. But I don't know this code or what it should be doing well enough to judge this at all. Inviting @tomaka to take a look instead.

@gnunicorn gnunicorn requested a review from tomaka December 16, 2019 17:14
#!/usr/bin/env sh
wasm-pack build --target web --out-dir ./browser-demo/pkg --no-typescript --release ./.. -- --no-default-features --features "browser"
python -m SimpleHTTPServer 8000
python -m http.server 8000
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Python2 is now deprecated 😅

@tomaka
Copy link
Contributor

tomaka commented Jan 2, 2020

IMO we should be more explicit in the keystore. The keystore should have a "memory-only" mode rather than using #[cfg] stuff. Once the keystore can be memory-only, we also no longer need to pass a dummy keystore path in the configuration.

What this PR does is okay-ish but doesn't go in the direction of having clean code.

We can either merge now and fix later, or improve this PR itself.

@expenses
Copy link
Contributor Author

expenses commented Jan 2, 2020

@tomaka What can be cleaned up?

@tomaka
Copy link
Contributor

tomaka commented Jan 2, 2020

What I mentioned: not having platform-specific code in the keystore, and the passing of a dummy path for the keystore field in the configuration.

}

/// Full client type.
pub type TFullClient<TBl, TRtApi, TExecDisp> = Client<
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, the next obvious step is to adjust config.keystore_path to be an enum of either a path or "in memory".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

LGTM. Obviously path being an Option is also not great, but I'm not going to ask you to entirely refactor Substrate in this PR.

@tomaka
Copy link
Contributor

tomaka commented Jan 3, 2020

This needs a Polkadot PR because of the change in the configuration now though.

@gavofyork
Copy link
Member

tests not passing either

@gnunicorn gnunicorn added A3-needsresolving and removed A0-please_review Pull request needs code review. labels Jan 6, 2020
@bkchr
Copy link
Member

bkchr commented Jan 6, 2020

Needs resolving.

@expenses
Copy link
Contributor Author

expenses commented Jan 6, 2020

Not sure what's causing wasm-bindgen-webidl to not compile - that version seems to work fine on my machine and the build server.

@expenses expenses merged commit 7f010fc into master Jan 7, 2020
@expenses expenses deleted the ashley-browser-utils branch January 7, 2020 15:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants