decode delegation query response fails in Compartment due to undefined Decimal #9408
Comments
|
I have reproduced the error locally but not yet isolated a defect. Given import { Decimal } from "@cosmjs/math";
import * as math from "@cosmjs/math";
console.log(Decimal);
console.log(math);
console.log(math.Decimal);I get the correct behavior from Node.js and incorrect behavior from import "ses";
import url from "url";
import fs from "fs";
import { importLocation } from "@endo/compartment-mapper";
import { makeReadPowers } from "@endo/compartment-mapper/node-powers.js";
const readPowers = makeReadPowers({ url, fs });
const location = new URL("isolate-me.js", import.meta.url).href;
importLocation(readPowers, location, {
globals: { console },
});The defect occurs at the boundary between an ESM/MJS module importing the CJS Given Object.defineProperty(exports, "__esModule", { value: true });
exports.b = void 0;
class B {}
exports.b = B;Given import { b } from "./b.cjs";
console.log(b);And this case works correctly in isolation under all of Node.js, importBundle, and importLocation. |
|
The culprit turns out to be the intermediate |
|
Thanks for the diagnosis! Our approach to endo compat so far has mostly been to patch the offending parts of cosmjs; for example, bn.js patch I'm not sure what would be the preferred tweak, but this seems to alleviate the symptoms: $ cat patches/@cosmjs+math+0.32.3.patch
diff --git a/node_modules/@cosmjs/math/build/index.js b/node_modules/@cosmjs/math/build/index.js
index 1f812f6..41f6398 100644
--- a/node_modules/@cosmjs/math/build/index.js
+++ b/node_modules/@cosmjs/math/build/index.js
@@ -2,7 +2,8 @@
Object.defineProperty(exports, "__esModule", { value: true });
exports.Uint64 = exports.Uint53 = exports.Uint32 = exports.Int53 = exports.Decimal = void 0;
var decimal_1 = require("./decimal");
-Object.defineProperty(exports, "Decimal", { enumerable: true, get: function () { return decimal_1.Decimal; } });
+// Object.defineProperty(exports, "Decimal", { enumerable: true, get: function () { return decimal_1.Decimal; } });
+exports.Decimal = decimal_1.Decimal;
var integers_1 = require("./integers");
Object.defineProperty(exports, "Int53", { enumerable: true, get: function () { return integers_1.Int53; } });
Object.defineProperty(exports, "Uint32", { enumerable: true, get: function () { return integers_1.Uint32; } });p.s. here's hoping for a |
refs: #9408 ## Description cosmjs deps add a lot of size and complicates our builds (#9583) At runtime we only need `Decimal` so this vendors that (just the subset necessary) and drops all other runtime deps. ### Security Considerations less supply chain ### Scaling Considerations none ### Documentation Considerations Maybe get this upstream to reduce maintenance. ### Testing Considerations CI suffices? ### Upgrade Considerations None, not on chain.
#2330) …S from set property of CJS Refs: Agoric/agoric-sdk#9408 ## Description If a modern JavaScript module imports a named binding from a CommonJS module, we can hardly guarantee that it will always work. Outrageous heroics make it work under most circumstances using a heuristic lexical analysis of the CommonJS module to determine which names should exist on the module’s internal namespace. Then, the CommonJS runtime must account for all variety of shenanigans that might cause the binding for that name to change. These include assignment to property names on `exports`, reassignment to `module.exports`, and `defineProperty` on the _original_ `exports` object. These changes must be reflected both on the module namespace object and on the `default` property thereof in _in every case where this is in fact possible_. Consider a CJS module (2.cjs) that exports a name: ```js exports.meaning = 42; ``` Then, consider a TypeScript module (1.ts) that reexports this name: ```ts export { meaning } from './2.cjs'; ``` Which gets compiled down to a CommonJS module (1.cjs): ```js const universe = require('./1.cjs'); Object.defineProperty(exports, 'meaning', { enumerable: true, get() { return universe.meaning } }); ``` Which gets imported by an ESM by name (0.mjs): ```js import { meaning } from './1.cjs'; ``` This has no right to work. We can trivially copy the getter from 1.cjs to the `default` export but the internal namespace object we use to emulate ESM in SES emits live binding notifications for every named property and doesn’t suffer `defineProperty` well. So, in the CommonJS emulation, we must attempt to propagate the current value from the getter to the module environment record after the module initializes. ### Security Considerations None. ### Scaling Considerations None. ### Documentation Considerations None. Folks already assume this works. It works in Node.js. Code exists that relies on it. ### Testing Considerations We have an existing test that exercises the case that the getter under a defineProperty can’t be expected to succeed on the stack of defineProperty. ### Compatibility Considerations This increases ecosystem compatibility. ### Upgrade Considerations None. ### PS It didn’t have to be like this.
Describe the bug
While working on
getDelegationsfor theunbondExample.contract.js, (#9070, #8863), I'm trying to useQueryDelegatorDelegationsResponse.decode(...)from@agoric/cosmic-protoinside a contract, but it fails due toDecimalfrom@cosmjs/mathbeing undefined.To Reproduce
I reduced it to running this in a compartment:
3aa40db has a test for that and a test for using
QueryDelegatorDelegationsResponse.decode(...).yarn; yarn buildcd packages/orchestrationyarn test test/tx-encoding.test.ts -m 'compartment*'Cannot read properties of undefinedyarn test run log
Expected behavior
QueryDelegatorDelegationsResponse.decode(...)works inside a compartment just like it does outside a compartment.Platform Environment
cc @kriskowal @turadg @0xpatrickdev
The text was updated successfully, but these errors were encountered: