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

fix(publish-kit): workaround endo bug #1728 #8232

Closed
wants to merge 1 commit into from

Conversation

erights
Copy link
Member

@erights erights commented Aug 22, 2023

Works around bug endojs/endo#1728 by omitting method guards for symbol-named methods.

Detected by #7937 as a consequence of the stricter checking just merged from endojs/endo#1715 . The InterfaceGuards created by this code were always incorrect in this manner. But previous error checking was too lax to catch it.

CI on this PR checks it against the endo normally used by agoric-sdk. #8233 checks this same change against "current" endo master, to see if this PR fixes the endojs/endo#1728 problem detected by #7937

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM as a workaround.

Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Should we instead fix this in endo, e.g. by updating makeInterfaceGuard?

 export const assertInterfaceGuard = specimen => {
   mustMatch(specimen, InterfaceGuardShape, 'interfaceGuard');
 };
 harden(assertInterfaceGuard);
+
+const { comparator: passableComparator } = makeComparatorKit();
 
 /**
  * @param {string} interfaceName
  * @param {Record<string, MethodGuard>} methodGuards
  * @param {{sloppy?: boolean}} [options]
  * @returns {InterfaceGuard}
  */
 const makeInterfaceGuard = (interfaceName, methodGuards, options = {}) => {
   const { sloppy = false } = options;
+  const stringMethodGuards = {};
+  const symbolMethodGuardsEntries = [];
+  for (const key of ownKeys(methodGuards)) {
+    const value = methodGuards[key];
+    if (typeof key === 'symbol') {
+      symbolMethodGuardsEntries.push([key, value]);
+    } else {
+      stringMethodGuards[key] = value;
+    }
+  }
+  // Ensure a consistent order.
+  symbolMethodGuardsEntries.sort(([k1], [k2]) => passableComparator(k1, k2));
   /** @type {InterfaceGuard} */
   const result = harden({
     klass: 'Interface',
     interfaceName,
-    methodGuards,
+    methodGuards: stringMethodGuards,
+    symbolMethodGuards: symbolMethodGuardsEntries,
     sloppy,
   });
   assertInterfaceGuard(result);
   return result;
 };

I'm not sure about backwards compatibility requirements here, but at the very least I think an endo issue is warranted.

@erights erights changed the title fix(publish-kit): workaround bug #8231 fix(publish-kit): workaround endo bug #1728 Aug 22, 2023
@erights
Copy link
Member Author

erights commented Aug 22, 2023

I'm not sure about backwards compatibility requirements here, but at the very least I think an endo issue is warranted.

Damn! I should have filed #8231 as an endo issue, not an agoric-sdk issue. GitHub does have a transfer issue feature, but seemingly only within the same org. Manually moved to endojs/endo#1728

@erights
Copy link
Member Author

erights commented Aug 22, 2023

Should we instead fix this in endo, e.g. by updating makeInterfaceGuard?

Yes. I like your suggestion of how. Since you've already written the code I'd start with, would you like to take this on? I'll review. Thanks!

Closing this PR in favor of that approach.

@erights
Copy link
Member Author

erights commented Aug 28, 2023

Closed in favor of endojs/endo#1740

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