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

move priceAuthorityRegistry with its vat #8717

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Jan 8, 2024

refs: #7399 (comment)

Description

priceAuthorityRegistry is in the Zoe package but it's the implementation of vat-priceAuthorityRegistry. This moves it adjacent to the latter.

It also has a misc typedef improvement that happened to be on my working branch.

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

This removes a file from the @agoric/zoe package, but it's in tools which is not part of the package API. So I didn't call it a breaking change. Also if I had used conventional commits to mark it a breaking change, it would have been called a breaking change to @agoric/vats too, which would be erroneous. One more reason to stop using conventional commits, imo.

@turadg turadg changed the title move priceAuthorityRegistry with its vat" move priceAuthorityRegistry with its vat Jan 8, 2024
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I defer approval to @Chris-Hibbert ; I think he's more swapped in on how to slice up the zoe package.

import { PriceAuthorityI } from '../src/contractSupport/priceAuthority.js';
import { PriceAuthorityI } from '@agoric/zoe/src/contractSupport/priceAuthority.js';
Copy link
Member

Choose a reason for hiding this comment

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

So the registry moves but not the priceAuthority API itself? OK.

It has always seemed strange to me that Price Authority docs are under zoe, but whatever.

Copy link
Member

Choose a reason for hiding this comment

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

The plan for deploying this change to production is the general "upgrade all vats" I suppose (#8104 -ish)

Copy link
Contributor

Choose a reason for hiding this comment

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

So the registry moves but not the priceAuthority API itself?

Regardless of where the priceAuthority API (and doc) winds up, the priceAuthorityRegistry reasonably belongs with the vat that its deployed in.

import { PriceAuthorityI } from '../src/contractSupport/priceAuthority.js';
import { PriceAuthorityI } from '@agoric/zoe/src/contractSupport/priceAuthority.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

So the registry moves but not the priceAuthority API itself?

Regardless of where the priceAuthority API (and doc) winds up, the priceAuthorityRegistry reasonably belongs with the vat that its deployed in.

import { providePriceAuthorityRegistry } from '../../../tools/priceAuthorityRegistry.js';
import { makeFakePriceAuthority } from '../../../tools/fakePriceAuthority.js';
import { providePriceAuthorityRegistry } from '../src/priceAuthorityRegistry.js';
import { makeFakePriceAuthority } from '../../zoe/tools/fakePriceAuthority.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to not import from @agoric/zoe? lint-primary is showing cross-package import as an error.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Jan 9, 2024
@turadg turadg force-pushed the ta/move-priceAuthorityRegistry branch 2 times, most recently from 6b30e98 to b70c02f Compare January 9, 2024 23:13
Copy link

@turadg - This PR appears to be stuck. It's had a merge label for > 24 hours

@turadg turadg force-pushed the ta/move-priceAuthorityRegistry branch from b70c02f to bf8a982 Compare January 11, 2024 16:11
Somewhat of a breaking change but it's moving from tools which is not part of the package API
@turadg turadg force-pushed the ta/move-priceAuthorityRegistry branch from bf8a982 to a92cf55 Compare January 11, 2024 16:32
@mergify mergify bot merged commit 53ec11c into master Jan 11, 2024
66 checks passed
@mergify mergify bot deleted the ta/move-priceAuthorityRegistry branch January 11, 2024 17:07
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants