polkadot-js / api

Promise and RxJS APIs around Polkadot and Substrate based chains via RPC calls. It is dynamically generated based on what the Substrate runtime provides in terms of metadata.
Apache License 2.0
1.07k stars 350 forks source link

Library compatibility with Secure EcmaScript #2062

Closed mpetrunic closed 3 years ago

mpetrunic commented 4 years ago

Secure EcmaScript proposal: https://github.com/tc39/proposal-ses

Steps to reproduce:

Only way we know how to reproduce is to run build on: https://github.com/NodeFactoryIo/metamask-snap-polkadot/tree/feature/connect-to-node

Error:

Build success: 'build/index.js' bundled as 'dist/bundle.js'!
Snap evaluation error: possible import expression rejected around line 8527

Code: image Which makes no sense as that line contains comment so it might be due to snap evaluation code.

mpetrunic commented 4 years ago

Opened issues on ses-shim repo in case it's bug there: https://github.com/Agoric/SES-shim/issues/234

jacogr commented 4 years ago

Thanks an absolute million for logging this - this is a good starting point to see what can be done. No promises, would need to dig.

So the specific code linked is actually as a result of the build process, and by that I mean not the API build process. That looks like follows for this file -

"use strict";

var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefault");

Object.defineProperty(exports, "__esModule", {
  value: true
});
var _exportNames = {
  Keyring: true,
  WsProvider: true,
  ApiPromise: true,
  ApiRx: true
};
Object.defineProperty(exports, "Keyring", {
  enumerable: true,
  get: function () {
    return _keyring.Keyring;
  }
});
Object.defineProperty(exports, "WsProvider", {
  enumerable: true,
  get: function () {
    return _rpcProvider.WsProvider;
  }
});
Object.defineProperty(exports, "ApiPromise", {
  enumerable: true,
  get: function () {
    return _promise.default;
  }
});
Object.defineProperty(exports, "ApiRx", {
  enumerable: true,
  get: function () {
    return _rx.default;
  }
});

var _util = require("@polkadot/util");

var _keyring = require("@polkadot/keyring");

var _rpcProvider = require("@polkadot/rpc-provider");

var _promise = _interopRequireDefault(require("./promise"));

var _rx = _interopRequireDefault(require("./rx"));

var _submittable = require("./submittable");

Object.keys(_submittable).forEach(function (key) {
  if (key === "default" || key === "__esModule") return;
  if (Object.prototype.hasOwnProperty.call(_exportNames, key)) return;
  Object.defineProperty(exports, key, {
    enumerable: true,
    get: function () {
      return _submittable[key];
    }
  });
});
// Copyright 2017-2020 @polkadot/api authors & contributors
// This software may be modified and distributed under the terms
// of the Apache-2.0 license. See the LICENSE file for details.
// FIXME This really should be `import(...).then(...)`, but need to check rejections
// eslint-disable-next-line @typescript-eslint/no-var-requires
(0, _util.detectPackage)(require('./package.json'), typeof __dirname !== 'undefined' && __dirname);
jacogr commented 4 years ago

So comparing your repo against the metamask base - swap to their gulp build process, non-snap.

mpetrunic commented 4 years ago

So snap build is using snaps-cli which contains build step which rewrites some code to be compatible with ses. We are looking into doing some preprocessing where we remove comments from code as it appears that ses evaluation fails on some import statements in comments (actual code uses requires). Hopefully someone from agoric will respond and give better insight into this.

mpetrunic commented 4 years ago

@jacogr It seems like initial errors were because SES evaluates comments as well. I've opened up PR on cli tool for building snaps which strip comments: https://github.com/MetaMask/snaps-cli/pull/50

Unfortunately, currently you cannot run wasm code inside ses. Is it possible to use this library without wasm initialization?

jacogr commented 4 years ago

I commented in substrate-technical -

It depends. So the wasm-crypto is basically a couple of things - faster versions of things like hashing and sr25519. For things like blake2, the slghtly-slower JS version will be used as a fallback when the wasm version is not avalable. sr25519 does not have any fallback, it is only wasm. (I'm using the term "wasm" here loosely, i.e. on platforms such as RN is uses asm.js instead of wasm, which is not available)

It always tries to instantiate the wasm portion, although it would be an easy addition to control that "auto-init-wasm" via a creation flag, aka don't use it, only use JS fallbacks. (Or even force to asm.js)

Specifically, we could control this via flag - https://github.com/polkadot-js/api/blob/master/packages/api/src/base/Init.ts#L220 which would allow all operations, except for anything with sr25519

jacogr commented 4 years ago

1.8.0-beta.10 allows the initWasm: false flag which would skip init on the API level itself. Please give it a try as see if it gets down the road somewhat. (As mentioned, not having wasm means no sr25519 support, which depending on use is ok or not)

The util-crypto will still try imports from wasm-crypto in the utils to determine which path to take, i.e. if wasm is available it will use that, otherwise fallback. If this creates an issue, there would need to be some magic in the util-crypto done.

So if the flag itself does not get over the line, the stub of wasm-crypto is the other approach. In the project package.json -

"browser": {
    "@polkadot/wasm-crypto": "./noWasm"
  },

Where the noWasm.js file has the following -

export function isReady () {
  // always false, when true it will try and use non-existent functions
  return false;
}

export function waitReady () {
  // always immediate true, our process is done
  return Promise.resolve(true);
}

(This is the hammer approach, even without the flag introduced it will just stub out the wasm completely, all ok for any hashing or ed25519 keys that can take either route)

mpetrunic commented 4 years ago

Yeah we are using ed25519 to avoid that. Tnx a lot for this. Will keep logging here if we encounter any more issues but it seems will manage to make it work :slightly_smiling_face:

jacogr commented 4 years ago

Please do add here, happy to see how we can get it pointing in the right direction.

mpetrunic commented 4 years ago

@jacogr We noticed that everytime we create and commentApiPromise, it makes a bunch of calls to fetch metadata. We can only store json serializable data in plugin state(we cannot really persist some global variable), so we ended up initializing ApiPromise on every snap invocation.

Question is, is it possible to extract metadata that can be serialized as json which we can pass somewhere when initializing api to avoid all those metadata calls and possibly make everything a bit faster?

jacogr commented 4 years ago

https://github.com/polkadot-js/api/blob/master/packages/api/src/types/index.ts#L40-L44

mpetrunic commented 4 years ago

We saw that, but from code it seems it would still fetch bunch of stuff like:

Second question do we pass ApiPromise.runtimeMetadata.toJSON() there?

jacogr commented 4 years ago

You don't pass the toJSON - you need the raw hex. Although the JSON may work now - but if you pass it manually, best is to get via

curl -H "Content-Type: application/json" -d '{"id":"1", "jsonrpc":"2.0", "method": "state_getMetadata", "params":[]}' http://localhost:9933

which will be valid for the same specVersion, as soon as upgraded it needs a bump, between specVersion changes the metadata cannot decode old anymore. (Or metadata.toHex() should also do the trick if you only have the object)

And it still needs to fetch the additional stuff, otherwise it has no guarantees that it is working with the correct metadata. (And with wrong metadata the API is completely stuck stuck everything uses the metadata. So it will, even when passing, still need to do some things)

mpetrunic commented 4 years ago

How long do you keep metadata cached then?

We need to serialize metadata to json to store it in plugin state(so we can reuse it on subsequent calls), if we need to pass Metadata object how do we restore Metadata from json so it's raw hex?

jacogr commented 4 years ago

Any Codec object has a .toHex() on it - https://polkadot.js.org/api/start/types.basics.html#everything-is-a-type

You can use it until the chain is upgraded and the specVersion has changed.

MakMuftic commented 4 years ago

@jacogr hi :wave: , we encountered another problem because of Secure EcmaScript doesn't allow setInterval function. After initializing API it disconnects after a few seconds (I suppose setInterval is used to keep the connection alive ?). For now, we are manually reconnecting using await provider.connect(); but definitely would like to do this more elegantly if possible. Do you think there is another way of solving this problem?

jacogr commented 4 years ago

Does it allow setTimeout?

I'll explain where this comes from - the actual connection does close without a health check when the subscriptions does not return any results in 30s. So the interval there is something that sends a system_health to keep the connection active.

So it depends on failure mode -

Either way, need some detection for these, or at worse a noKeepalive flag?

So I guess the question is which options is availble here, and is it undefined or just throws on explicit use?

jacogr commented 3 years ago

Closing. Feel free to re-open if there are more.

polkadot-js-bot commented 3 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.