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

chore: Sync Endo #8651

Merged
merged 17 commits into from
Dec 21, 2023
Merged

chore: Sync Endo #8651

merged 17 commits into from
Dec 21, 2023

Conversation

kriskowal
Copy link
Member

@kriskowal kriskowal commented Dec 12, 2023

This change brings Agoric SDK in sync with the latest versions from Endo as of December 12, 2023.

This includes syncing up breaking changes with interface guards. Where invalid call guards would silently allow an invocation previously, they now throw. Several interface guards needed fixing.

Some accommodations were necessary for the new __getMethodNames__ and __getInterfaceGuard__ protocols.

The algorithm for performing a speculative integration with an Endo branch required an adjustment to ensure that types were generated correctly.

For reasons opaque to the integrator, this required:

  • many type imports to qualify the .js extension. The only relevant change from the Endo side is that many packages provide generated .d.ts types now.
  • @agoric/timeto have atypesfield pointing atindex.js`, which by dint of fortunate coïncidence is a valid TypeScript definition file.
  • a type error to be suppressed in @agoric/zone that only causes a type error in integration with @agoric/vats.

@kriskowal kriskowal mentioned this pull request Dec 12, 2023
@kriskowal kriskowal added the force:integration Force integration tests to run on PR label Dec 12, 2023
@kriskowal kriskowal marked this pull request as ready for review December 12, 2023 21:23
packages/zoe/test/unitTests/test-zoe.js Outdated Show resolved Hide resolved
getRoundData: M.call(M.any()).returns(M.promise()),
getRoundStatus: M.call(M.any()).returns(M.record()),
oracleRoundStateSuggestRound: M.call(M.any()).returns(M.record()),
authenticateQuote: M.call().rest(M.any()).returns(M.any()),
Copy link
Member

Choose a reason for hiding this comment

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

This should take one quote,

/** @param {PriceQuoteValue} quote */
const authenticateQuote = quote => {
const quoteAmount = AmountMath.make(brand, harden(quote));
const quotePayment = quoteMint.mintPayment(quoteAmount);
return harden({ quoteAmount, quotePayment });
};
const calcAmountOut = amountIn => {

I'll push accurate guards.

Copy link
Member

Choose a reason for hiding this comment

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

Better guards would certainly be nice, but can happen after this PR. Thus, I won't withhold approval waiting for this conversation to be resolved.

Copy link
Member

Choose a reason for hiding this comment

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

refactor: Make all TS import extensions explicit

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tsc made me do it. I do not know why tsc insisted, but it did.

Comment on lines 30 to 31
* @typedef {import('@agoric/time').RelativeTime} RelativeTime // TODO: use
* RelativeTime, not RelativeTimeValue
Copy link
Member

Choose a reason for hiding this comment

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

this TODO is odd. perhaps it's meant to be a deprecated type?

@@ -89,6 +89,10 @@ export const makeDurableZone = (baggage, baseLabel = 'durableZone') => {
/** @type {import('.').Zone['exoClass']} */
const exoClass = (...args) => prepareExoClass(baggage, ...args);
/** @type {import('.').Zone['exoClassKit']} */
// @ts-ignore This type check regressed inexplicably with the release
Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look at fixing this before approving

Copy link
Member

Choose a reason for hiding this comment

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

Likewise, although it would be nice to get this resolved now, it can happen after this PR, so I won't withhold approval waiting for it.

package.json Outdated
@@ -10,7 +10,7 @@
"type": "module",
"packageManager": "[email protected]",
"devDependencies": {
"@endo/eslint-plugin": "^0.5.1",
"@endo/eslint-plugin": "^1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

these 1.0s, will they all be semver? Are Endo maintainers committed to major very bump for all breaking changes? do deep imports count as the package API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Deep imports are expressly forbidden by an "exports" directive in every public Endo package except captp, far, lockdown, and marshal, and these must be fixed.

Endo has for some time committed to backward-compatibility for dependents using the default carat (^) range, which is more strict than semver.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Tracking the broader issue for Agoric SDK #8679

@erights
Copy link
Member

erights commented Dec 12, 2023

This change brings Agoric SDK in sync with the latest versions from Endo as of December 12, 2003.

Wow, that's a long time ago!

@kriskowal kriskowal force-pushed the kris-sync-endo-2023-12-12-20-31-15 branch 8 times, most recently from 3d3a6f3 to 89c2b4b Compare December 20, 2023 21:41
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM!!!

getRoundData: M.call(M.any()).returns(M.promise()),
getRoundStatus: M.call(M.any()).returns(M.record()),
oracleRoundStateSuggestRound: M.call(M.any()).returns(M.record()),
authenticateQuote: M.call().rest(M.any()).returns(M.any()),
Copy link
Member

Choose a reason for hiding this comment

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

Better guards would certainly be nice, but can happen after this PR. Thus, I won't withhold approval waiting for this conversation to be resolved.

@@ -89,6 +89,10 @@ export const makeDurableZone = (baggage, baseLabel = 'durableZone') => {
/** @type {import('.').Zone['exoClass']} */
const exoClass = (...args) => prepareExoClass(baggage, ...args);
/** @type {import('.').Zone['exoClassKit']} */
// @ts-ignore This type check regressed inexplicably with the release
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, although it would be nice to get this resolved now, it can happen after this PR, so I won't withhold approval waiting for it.

@kriskowal kriskowal force-pushed the kris-sync-endo-2023-12-12-20-31-15 branch from 89c2b4b to 69884dd Compare December 21, 2023 01:34
@kriskowal kriskowal added the automerge:rebase Automatically rebase updates, then merge label Dec 21, 2023
@mergify mergify bot merged commit 4f72580 into master Dec 21, 2023
72 of 79 checks passed
@mergify mergify bot deleted the kris-sync-endo-2023-12-12-20-31-15 branch December 21, 2023 02:08
This was referenced Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants