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

refactor(zoe): Replace arrayToObj with objectMap #8856

Merged
merged 4 commits into from
Feb 11, 2024

Conversation

gibson042
Copy link
Member

@gibson042 gibson042 commented Feb 2, 2024

Description

arrayToObj is an old function and has significant overlap with objectMap (the latter is basically Object.fromEntries(Object.entries(obj).map(([key, value]) => [key, mapFn(value, key)])), and the former is basically Object.fromEntries(zip(keys, values)) where values comes from manually mapping over original values).

This PR removes it in favor of more idiomatic code.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

Probably none; no new code is using the affected functions.

Testing Considerations

All remaining tests should continue to pass.

Upgrade Considerations

n/a; behavior is unaffected

@gibson042 gibson042 added enhancement New feature or request Zoe package: Zoe code-style defensive correctness patterns; readability thru consistency labels Feb 2, 2024
@gibson042 gibson042 marked this pull request as draft February 2, 2024 20:51
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.

I'll wait until this is ready for review before doing a more careful read. But looks good to me so far. A welcome simplification!

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.

Please do fix all the imports of this. Thanks.

packages/zoe/src/issuerStorage.js Show resolved Hide resolved
@gibson042
Copy link
Member Author

There is a test failure in dapp-card-store integration caused by promise chains internal to deeplyFulfilled via E.when. It should be resolved by Agoric/dapp-card-store#73 .

@gibson042 gibson042 marked this pull request as ready for review February 3, 2024 01:17
@gibson042 gibson042 force-pushed the gibson-2024-02-remove-arraytoobj branch 2 times, most recently from e196e2a to 2006618 Compare February 3, 2024 02:22
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

@gibson042 gibson042 force-pushed the gibson-2024-02-remove-arraytoobj branch from 7ca9f5d to 726c44d Compare February 3, 2024 16:40
@gibson042
Copy link
Member Author

@erights: I added a new commit relating to yesterday's discussion: 726c44d

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Looks mechanical to me.

@erights
Copy link
Member

erights commented Feb 5, 2024

@erights: I added a new commit relating to yesterday's discussion: 726c44d

The disjointness of keywords will remain. So under that assumption, this indeed has no observable change. But it is more robust to possible future design changes as well as lack-of-enforcement errors.

Re "yesterday's discussion", I think that had to do with a restriction on having the "same" asset in give and want, for some def of "same" I cannot recall. In any case, @Chris-Hibbert is not finding that so my memory is likely confused. In any case, the problem we wanted to solve by lifting this (possibly non-existent) check would not be addressed by relaxing the disjoint keyword prohibition. They're independent.

In any case, still LGTM. Please proceed.

…before give properties

This currently has no significant effect because cleanProposal uses
assertKeywordNotInBoth to guarantee disjoint keywords, but we should not
overwrite give amounts when/if that constraint is removed.
@gibson042 gibson042 force-pushed the gibson-2024-02-remove-arraytoobj branch from 726c44d to 2165e1e Compare February 10, 2024 20:56
@gibson042 gibson042 added the automerge:rebase Automatically rebase updates, then merge label Feb 11, 2024
@mergify mergify bot merged commit cd8e0aa into master Feb 11, 2024
75 checks passed
@mergify mergify bot deleted the gibson-2024-02-remove-arraytoobj branch February 11, 2024 16:36
@Chris-Hibbert
Copy link
Contributor

Most of this is a clean refactor. The code added in 726c44d looks like a semantic change. I'm going to add it to my list to be included in upgrade 16.

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 code-style defensive correctness patterns; readability thru consistency enhancement New feature or request Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants