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.

Sign up for GitHub

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

Jump to bottom

refactor: Add mailbox types and differentiate use of maps from map-like stores #10290

Merged
merged 1 commit into from
Oct 17, 2024
Merged

refactor: Add mailbox types and differentiate use of maps from map-like stores #10290

merged 1 commit into from
Oct 17, 2024

Conversation

gibson042
Copy link
Member

(split from #10165)

Description

chain-main.js passes a non-map mailboxStorage to launch, which threads it into mailbox code that returns methods expecting a map-like entries method and size property. This PR adds type data to reject such issues and refactors mailbox code accordingly by converting those methods into independent functions that accept appropriate backing maps.

Security Considerations

None known.

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

Existing tests should continue to pass.

Upgrade Considerations

This code does not affect any vat and is safe to include in the next release.

@gibson042 gibson042 requested a review from a team as a code owner October 16, 2024 19:02
turadg
turadg approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Happy to see more types in the kernel.

LGTM once you fix the outbox def.

@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented Oct 16, 2024 •
edited
Loading

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6822daf
Status: ✅  Deploy successful!
Preview URL: https://16c8dd53.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-2024-10-mailbox-types.agoric-sdk.pages.dev

View logs

@mhofman
Copy link
Member

mhofman commented Oct 17, 2024

I plan on reviewing this, but I'm surprised to see a "breaking change" ! on a change tagged as a refactor.

@gibson042
Copy link
Member Author

I plan on reviewing this, but I'm surprised to see a "breaking change" ! on a change tagged as a refactor.

Yeah, I'm not sure how to accurately account for the details here—the return value of buildMailboxStateMap(state) (which is used to build the currently-unused mailbox device) is losing exportToData and populateFromData methods, because the former relies on state.entries() and the latter on state.size despite the fact that launch-chain.js provides a state that is a KVStore backed by vstorage rather than a map (and specifically lacks both entries and size). So replacing those methods with independent functions is technically a breaking change, but the on-chain code could never have used them anyway. I'm open to ideas on how to communicate that in a better way than refactor!.

@turadg
Copy link
Member

turadg commented Oct 17, 2024

technically a breaking change, but the on-chain code could never have used them anyway. I'm open to ideas on how to communicate that in a better way than refactor!.

refactor! is often what I reach for in a breaking change in naming but I agree it's a misuse of the term. chore! is probably the valid conventional commit.

IMO, if you are confident this couldn't actually break consumers then it's not necessary to mark.

Another reason not to have ! is because the commit affects three packages and they certainly don't all have a breaking change.

mhofman
mhofman approved these changes Oct 17, 2024
View reviewed changes
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm confused why we need to add a getNextKey on the mailbox storage of boot swingset tools.

packages/boot/tools/supports.ts Outdated
Comment on lines 516 to 523
/** @type {KVStore<Mailbox>} */
const mailboxStorage = Object.assign(new Map(), {
getNextKey: previousKey => {
for (const k of mailboxStorage.keys()) {
if (k > previousKey) return k;
}
},
});
Copy link
Member

Choose a reason for hiding this comment

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

yikes, why do we need this ugly inefficient appendage? And why wasn't it needed before? If it's just to satisfy types, can we add a "not implemented" throwing implementation like

agoric-sdk/packages/cosmic-swingset/src/helpers/bufferedStorage.js

Lines 174 to 176 in 55a8847

getNextKey(_previousKey) {
throw Error('not implemented');
},

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, now simplified to just a ts-expect-error.

@mhofman
Copy link
Member

mhofman commented Oct 17, 2024

IMO, if you are confident this couldn't actually break consumers then it's not necessary to mark.

Another reason not to have ! is because the commit affects three packages and they certainly don't all have a breaking change.

My understanding is that it's not actually breaking anyone. Let's drop the !

@gibson042
refactor: Add mailbox types and differentiate use of maps from map-li…
6822daf 
…ke stores

Notably, chain-main.js passes a non-map mailboxStorage to `launch`,
which threads it into mailbox code.
@gibson042 gibson042 force-pushed the gibson-2024-10-mailbox-types branch from 4128aef to 6822daf Compare October 17, 2024 19:52
@gibson042 gibson042 changed the title refactor!: Add mailbox types and differentiate use of maps from map-like stores refactor: Add mailbox types and differentiate use of maps from map-like stores Oct 17, 2024
@gibson042 gibson042 added the automerge:rebase Automatically rebase updates, then merge label Oct 17, 2024
@mergify mergify bot merged commit d322e42 into master Oct 17, 2024
91 checks passed
@mergify mergify bot deleted the gibson-2024-10-mailbox-types branch October 17, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turadg turadg turadg approved these changes

@mhofman mhofman mhofman approved these changes

@warner warner Awaiting requested review from warner

Assignees
No one assigned
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gibson042 @mhofman @turadg