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

Improved Store API #4191

Merged
merged 2 commits into from
Jan 6, 2022
Merged

Improved Store API #4191

merged 2 commits into from
Jan 6, 2022

Conversation

erights
Copy link
Member

@erights erights commented Dec 19, 2021 •
edited
Loading

Changes to #4190 that go beyond extraction from #4136 plus minor cleanups. These changes should be reviewed separately. If merged, they would require more substantial changes to #4136.

All the key, pattern, copySet, and copyMap stuff have been extracted into the earlier #4210. The differential left here is only about stores.

@erights erights requested a review from FUDCo December 19, 2021 19:53
@erights erights self-assigned this Dec 19, 2021
@erights erights changed the title fix: unnest makePatternKit Proposed improvements to 4136 Dec 19, 2021
@erights erights mentioned this pull request Dec 20, 2021
Closed
@erights erights force-pushed the markm-4136-extract-early branch from 0f9df16 to 733b2cd Compare December 21, 2021 01:34
@erights erights force-pushed the markm-proposed-4136-improvements branch from 8956174 to ab60559 Compare December 21, 2021 03:28
@erights erights force-pushed the markm-4136-extract-early branch from 75f88e3 to 0f0b1ea Compare December 23, 2021 00:56
Base automatically changed from markm-4136-extract-early to master December 23, 2021 01:11
@erights erights force-pushed the markm-proposed-4136-improvements branch 4 times, most recently from 4c12b99 to ae2ea31 Compare December 24, 2021 06:31
@erights erights mentioned this pull request Dec 24, 2021
Merged
@erights erights changed the base branch from master to markm-extract-4191-early December 24, 2021 07:48
@erights erights force-pushed the markm-proposed-4136-improvements branch 2 times, most recently from 5e38fca to b45c364 Compare December 24, 2021 08:33
@erights erights changed the title Proposed improvements to 4136 Improved Store API Dec 25, 2021
@erights erights mentioned this pull request Dec 25, 2021
Merged
@erights erights force-pushed the markm-proposed-4136-improvements branch from b45c364 to 49c0417 Compare December 25, 2021 20:25
@erights erights force-pushed the markm-extract-4191-early branch 5 times, most recently from 3899a51 to 9a58425 Compare December 28, 2021 03:36
@erights erights force-pushed the markm-proposed-4136-improvements branch from 49c0417 to 0f75e65 Compare December 28, 2021 03:51
@erights erights force-pushed the markm-extract-4191-early branch from c35d285 to 4a7cbdc Compare December 30, 2021 00:50
@erights erights force-pushed the markm-proposed-4136-improvements branch from 0f75e65 to e14c93e Compare December 30, 2021 00:52
@erights erights force-pushed the markm-extract-4191-early branch from 4a7cbdc to b340a81 Compare January 3, 2022 21:50
Base automatically changed from markm-extract-4191-early to master January 3, 2022 22:03
FUDCo
FUDCo approved these changes Jan 3, 2022
View reviewed changes
Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

Once again, this looks fine, but there's a lot of details and so could benefit from an additional set of eyes.

@erights erights force-pushed the markm-proposed-4136-improvements branch from e14c93e to 5fd1172 Compare January 3, 2022 22:24
@erights erights requested a review from Chris-Hibbert January 4, 2022 00:24
@erights
Copy link
Member Author

erights commented Jan 4, 2022

Hi @Chris-Hibbert , we need your eyeballs ;). Thanks.

@erights erights force-pushed the markm-proposed-4136-improvements branch from 5fd1172 to a4e19e3 Compare January 4, 2022 01:36
Chris-Hibbert
Chris-Hibbert reviewed Jan 4, 2022
View reviewed changes
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

Everything that I read and understood (other than the word contact in some comments) looked good to me.
I asked some questions for my own edification.

I didn't completely understand the changes in the various stores. If that was the review that you wanted, I can take another look and dive deeper into the semantics.

packages/governance/src/electorateTools.js Show resolved Hide resolved
packages/governance/src/paramGovernance/paramManager.js Show resolved Hide resolved
packages/governance/src/paramGovernance/paramManager.js Show resolved Hide resolved
packages/wallet/api/src/internal-types.js Show resolved Hide resolved
packages/zoe/src/contractFacet/zcfSeat.js Show resolved Hide resolved
@samsiegart
Copy link
Contributor

Just tested card-store, treasury, and fungible faucet and they all still work with this change (though I did file some unrelated issues that I found). I'll follow up when I get loadgen running successfully to confirm that it doesn't break.

@samsiegart
Copy link
Contributor

@mhofman Is working on automated tests for the loadgen and getting it to work with previous sdk changes, so not a blocker for now. All manual dapp testing is 🟢

@mhofman
Copy link
Member

mhofman commented Jan 6, 2022

I manually ran this branch against the loadgen and didn't encounter any issues.

@erights erights force-pushed the markm-proposed-4136-improvements branch from a4e19e3 to 7f06770 Compare January 6, 2022 21:37
@erights erights added the automerge:squash Automatically squash merge label Jan 6, 2022
@mergify mergify bot merged commit 9ba09ec into master Jan 6, 2022
@mergify mergify bot deleted the markm-proposed-4136-improvements branch January 6, 2022 22:31
@michaelfig michaelfig mentioned this pull request Jan 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chris-Hibbert Chris-Hibbert Chris-Hibbert left review comments

@FUDCo FUDCo FUDCo approved these changes

Assignees

@erights erights

Labels
automerge:squash Automatically squash merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erights @samsiegart @mhofman @FUDCo @Chris-Hibbert