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

[Zoe] Allow sparse keywords #812

Merged
merged 1 commit into from
Apr 4, 2020
Merged

[Zoe] Allow sparse keywords #812

merged 1 commit into from
Apr 4, 2020

Conversation

katelynsills
Copy link
Contributor

@katelynsills katelynsills commented Mar 31, 2020

In reviewing, let's make sure this implementation matches the "spec" outlined in this issue: #800

Closes #800
Closes #393
Closes #785
Closes #849

Closes partially #847

@katelynsills katelynsills marked this pull request as ready for review April 1, 2020 21:55
@katelynsills katelynsills force-pushed the 800-sparse-keywords branch 2 times, most recently from 9834f8c to 912bdb7 Compare April 1, 2020 21:56
@katelynsills katelynsills changed the title [WIP] sparse keywords [Zoe] Allow sparse keywords Apr 1, 2020
packages/zoe/src/cleanProposal.js Outdated Show resolved Hide resolved
packages/zoe/src/cleanProposal.js Outdated Show resolved Hide resolved
packages/zoe/src/contracts/autoswap.js Show resolved Hide resolved
packages/zoe/src/objArrayConversion.js Outdated Show resolved Hide resolved
packages/zoe/src/objArrayConversion.js Outdated Show resolved Hide resolved
// Return the keys after asserting this.
const checkKeys = (expectedKeys, record) => {
// Assert that keys, if present, match expectedKeys.
export const assertCapASCII = keyword => {
Copy link
Contributor

Choose a reason for hiding this comment

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

cleanProposal.js is exporting a few things that aren't covered by the current name. Would it be better to rename it, or to split its functions up further?

The comment above no longer covers the file, so it should be moved closer to the function it describes. (Or the fn moved up here if it doesn't depend on the other fns.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure how to split up the content of this file versus objArrayConversion.js. They are both more generalized helpers now.

I will move that comment!

packages/zoe/src/zoe.js Outdated Show resolved Hide resolved
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment