refactor: move atomicRearrange into Zcf #7900

Chris-Hibbert · 2023-06-08T21:34:12Z

Description

This is the next step of #6577 (atomic reallocations for contracts). It adds native support in ZCF for atomicReallocate, which makes it possible to update contracts from the helper to use the ZCF-native approach, which is a prerequisite for removing the old hazardous staging-based approach.

This PR only affects ZCF, though it updates types referenced from inter-protocol contracts. This is required in order to keep the mono-repo consistent, and has no run-time impact on the contracts, even if changes are pushed to some live contracts even before the ZOE change takes effect.

The sample contracts in Zoe have also been updated to use the new API. These do not affect the interprotocol contracts. The only zoe contract referenced by inter-protocol contracts is scaledPriceAuthority, which is not changed here.

The new code was tested with versions of the inter-protocol contracts updated to use the ZCF-native API. Those contracts were reverted to their old form before merging this PR since the contracts should be updated separately from Zoe. None of the contracts should be updated to use the new reallocation API until this PR is running on chain. They can be individually updated once this code is in place.

This PR deprecates all the parts of the old approach, as well as the interim helper. (reallocate, getStagedAllocation, atomicRearrange, incrementBy, decrementBy, and hasStagedAllocation.)

Security Considerations

A step toward removing hazard-prone reallocation via staging.

Scaling Considerations

None.

Documentation Considerations

Document the deprecations and the new approach to reallocation.

Testing Considerations

Validated all the existing contracts (zoe examples, as well as Inter-prorotocol contracts) work with the new approach. The Zoe examples are updated with this PR. The Inter-protocl contracts were reverted, and continue to use the helper.

Added new tests for the function that coverts TransferPart descriptions to new allocations.

Release considerations

This PR adds an API to ZCF within Zoe. It can be released by upgrading the Zoe contract. Existing contracts will continue to work after this change. After this change, individual contracts can be upgraded to migrate to the new API.

Chris-Hibbert · 2023-06-08T23:22:25Z

@turadg, you are the primary reviewer.
@erights, FYI.
@warner, FYI in the context of writing up release plans for PRs that update code on chain.

packages/zoe/src/contractFacet/types.js

packages/zoe/src/contractFacet/reallocate.js

packages/zoe/src/contractFacet/types.js

turadg · 2023-06-09T15:02:08Z

packages/zoe/src/contractFacet/types.js

@@ -184,6 +197,14 @@
 * @returns {Amount<any>}
 */

+/**
+ * @deprecated Use atomicRearrange instead


I think JSDoc does not support deprecating callbacks. In VSCode there's no strikethrough like other deprecations. I think because incrementBy property itself isn't deprecated. The notice is from line 223.

To mark these two properties as deprecated we'll have to use .ts syntax for the types. Unfortunately, that makes is harder to share them ambiently. We could put them in the global namespace but that's not technically correct.

Alternately, we could define the ZCFSeat type using the concrete implementation, which can also mark things deprecated. I'm fine deferring that.

Meanwhile, please revert making DeprecatedIncrementDecrementBy as it doesn't do anything :)

Thanks for checking this, and looking for other alternatives. It annoyed me that I couldn't find a way to get a strike-through on incrementBy and decrementBy.

turadg · 2023-06-09T15:05:29Z

packages/zoe/src/contractFacet/zcfSeat.js

@@ -214,6 +216,10 @@ export const createSeatManager = (

        return isOfferSafe(state.proposal, reallocation);
      },
+      /**
+       * @deprecated switch to zcf.atomicRearrange()


This is good but note that consumers of this object aren't getting the type written here. They get ZCFSeat which has the problem discussed above.

Agreed. The comment is mostly for those reading/editing this file. This still seemed the most concise way to express it.

packages/zoe/src/contractFacet/zcfSeat.js

packages/zoe/test/unitTests/zcf/test-atomicRearrange.js

packages/zoe/src/contractFacet/zcfSeat.js

erights · 2023-06-23T03:30:10Z

packages/zoe/src/contractSupport/atomicTransfer.js

@@ -52,6 +43,8 @@ export const TransferPartShape = M.splitArray(
 * which will remain helpers. These helper are for convenience
 * in expressing atomic rearrangements clearly.
 *
+ * @deprecated use the zcf builtin instead
+ *
 * @param {ZCF} zcf
 * @param {TransferPart[]} transfers
 */


If the new code has the same semantics, why not redefine the old code to delegate to the new code?

Suggested change

*/

*/

export const atomicRearrange = (zcf, transfers) => zcf.atomicRearrange(transfers);

Doh! of course.

packages/zoe/src/contractFacet/reallocate.js

turadg · 2023-06-23T21:42:00Z

packages/zoe/src/contractFacet/reallocate.js

+  }
+
+  /** @type {[ZCFSeat,AmountKeywordRecord][]} */
+  const resultingAllocations = [];


resultingAllocations is a different shape than allocations. consider renaming to resultingEntries or resolvedEntries.

consider a functional style using a transform function:

const resolveAllocation = (seat, [incrList, decrList]) => {…}; const resolvedEntries = allocations.entries().map(resolveAllocation);

I think I looked at that, but allocations.entries() isn't an array. Using Array.from() would require declaring more types.

packages/zoe/src/contractFacet/types.js

packages/zoe/src/contractFacet/zcfSeat.js

turadg · 2023-06-23T22:02:06Z

packages/zoe/src/contractFacet/zcfSeat.js

+            ([seat, allocation]) => {
+              const seatHandle = zcfSeatToSeatHandle.get(seat);
+              return { seatHandle, allocation };
+            },


nit for concision in map callbacks

Suggested change

([seat, allocation]) => {

const seatHandle = zcfSeatToSeatHandle.get(seat);

return { seatHandle, allocation };

},

([seat, allocation]) => ({

{ allocation, seatHandle: zcfSeatToSeatHandle.get(seat) };

}),

done.

It doesn't seem any more concise to me. If prettier had plausible (to me, I admit) rules, it would be better as you show, but prettier formats it as

([seat, allocation]) => ({ allocation, seatHandle: zcfSeatToSeatHandle.get(seat), }),

Which is not an improvement, AFAICT. In fact, seeing this, I might extract seatHandle so the return value will be on one line.

packages/zoe/src/contractFacet/zcfSeat.js

turadg · 2023-06-23T22:08:40Z

packages/zoe/src/contractFacet/zcfSeat.js

+              activeZCFSeats.set(seat, allocation),
+            );
+
+            E(zoeInstanceAdmin).replaceAllocations(seatHandleAllocations);


this is a promise that may reject. if so it will fail silently.

I see that reallocate had this same problem. Am I missing something?

We have tried to ensure that ZCF has the authoritative word here. If ZCF and Zoe lose contact then things are very broken and will shortly fall apart.

Short of that, Zoe and ZCF are in close communication and we have tried to ensure that the two sides remain in sync, and for every shared bit of info, we know which side is authoritative.

Ok. Please put void and a comment with that justification. In particular, why a failure of replaceAllocations shouldn't be caught by the catch and shutdownWithFailure.

erights · 2023-07-16T01:08:35Z

Just re-requested a review from myself to make this easier for me to find.

@ivanlei due to finger error, I also re-requested a review from you. I don't know how to undo that.

This is the next step of #6577 (atomic reallocations for contracts). It adds native support in ZCF for atomicReallocate, which makes it possible to update contracts from the helper to use the ZCF-native approach, which is a prerequisite for removing the old hazardous staging-based approach. This PR only affects ZCF, though it updates types referenced from inter-protocol contracts. This is required in order to keep the mono-repo consistent, and has no run-time impact on the contracts, even if changes are pushed to some live contracts even before the ZOE change takes effect. The sample contracts in Zoe have also been updated to use the new API. These do not affect the interprotocol contracts. The only zoe contract referenced by inter-protocol contracts is scaledPriceAuthority, which is not changed here. The new code was tested with versions of the inter-protocol contracts updated to use the ZCF-native API. Those contracts were reverted to their old form before merging this PR since the contracts should be updated separately from Zoe. None of the contracts should be updated to use the new reallocation API until this PR is running on chain. They can be individually updated once this code is in place.

turadg

One small change requested.

Let's also discuss whether this can land before its upgrade test or should be bundled with it.

turadg

🚢

refactor: move atomicRearrange into Zcf

refs: #6678 refs: #7900 ## Description update VaultFactory to use zcf.atomicRearrange. This change **must not** be pushed to the chain before #7900. ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations None ### Release considerations This requires a change to Zoe (#7900). Since VaultFactory gets upgraded separately from Zoe, this is staged as a separate PR. Once #7900 is on chain, this update can be made, even if it's in the same release cycle, as long as Zoe is upgraded first. This change is independent of changes to other contracts.

refs: #6678 refs: #7900 ## Description update Auction to use `zcf.atomicRearrange()`. This change must not be pushed to the chain before #7900. ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations None ### Release considerations This requires a change to Zoe (#7900). Since the Auction gets upgraded separately from Zoe, this is staged as a separate PR. Once #7900 is on chain, this update can be made, even if it's in the same release cycle, as long as Zoe is upgraded first. This change is independent of changes to other contracts.

closes: #6678 refs: #7900 ## Description update PSM to use `zcf.atomicRearrange()`. This change must not be pushed to the chain before #7900. ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations None ### Release considerations This requires a change to Zoe (#7900). Since PSM gets upgraded separately from Zoe, this is staged as a separate PR. Once #7900 is on chain, this update can be made, even if it's in the same release cycle, as long as Zoe is upgraded first. This change is independent of changes to other contracts.

Chris-Hibbert added Zoe package: Zoe Zoe Contract Contracts within Zoe labels Jun 8, 2023

Chris-Hibbert self-assigned this Jun 8, 2023

Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch from b6e326b to 561dadf Compare June 8, 2023 21:40

Chris-Hibbert marked this pull request as draft June 8, 2023 21:49

Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch from 561dadf to a178f26 Compare June 8, 2023 21:54

Chris-Hibbert mentioned this pull request Jun 8, 2023

refactor(vaultFactory!): vaultFactory use zcf.atomicRearrange #7901

Merged

Chris-Hibbert requested review from turadg and erights June 8, 2023 23:20

Chris-Hibbert marked this pull request as ready for review June 8, 2023 23:20

This was referenced Jun 8, 2023

refactor(PSM!): make PSM use zcf.atomicRearrange() #7903

Merged

refactor(Auction!): make Auction use zcf.atomicRearrange() #7904

Merged

erights reviewed Jun 8, 2023

View reviewed changes

packages/zoe/src/contractFacet/types.js Outdated Show resolved Hide resolved

turadg requested changes Jun 9, 2023

View reviewed changes

Chris-Hibbert mentioned this pull request Jun 12, 2023

Allow core-eval to update Zoe's choice of ZCF bundle when starting new contracts #7918

Closed

Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch 2 times, most recently from b8f88d7 to 14ac202 Compare June 22, 2023 16:38

Chris-Hibbert mentioned this pull request Jun 22, 2023

remove reallocate from ZCF #6678

Closed

Chris-Hibbert requested a review from turadg June 22, 2023 22:38

erights reviewed Jun 23, 2023

View reviewed changes

Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch from fbe162c to a7a4fed Compare June 23, 2023 21:05

turadg requested changes Jun 23, 2023

View reviewed changes

turadg mentioned this pull request Jun 23, 2023

V+1 contract changes #7982

Closed

Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch from a7a4fed to a16493d Compare June 23, 2023 23:33

erights requested review from erights and ivanlei July 16, 2023 01:07

turadg removed the request for review from ivanlei July 17, 2023 15:54

Chris-Hibbert added 4 commits July 17, 2023 15:52

test: exclude test-atomicRearrange from XS testing

ff54e41

chore: remove unhelpful type and default declaration

03ac2cd

chore: simplify remain atomicRearrange helper; use forEach over map

f54a73b

chore: local review clean-ups

b9c3ff2

Chris-Hibbert force-pushed the 6678-intrinsicAtomicTransfer branch from a16493d to b9c3ff2 Compare July 17, 2023 23:07

Chris-Hibbert requested a review from turadg July 17, 2023 23:53

turadg reviewed Jul 21, 2023

View reviewed changes

doc: add void label on remote call to replaceAllocatios() & document

8de8500

Chris-Hibbert enabled auto-merge July 26, 2023 20:37

Chris-Hibbert requested a review from turadg July 26, 2023 20:38

Merge branch 'master' into 6678-intrinsicAtomicTransfer

5a00ae1

turadg approved these changes Jul 27, 2023

View reviewed changes

Chris-Hibbert added this pull request to the merge queue Jul 27, 2023

Merged via the queue into master with commit cc54325 Jul 27, 2023

Chris-Hibbert deleted the 6678-intrinsicAtomicTransfer branch July 27, 2023 19:37

This was referenced Jul 28, 2023

test: a test framework for verifying upgrade of Zoe and ZCF #7969

Closed

move atomicTransfer from helper into Zoe #6679

Open

mhofman pushed a commit that referenced this pull request Aug 7, 2023

Merge pull request #7900 from Agoric/6678-intrinsicAtomicTransfer

1f73195

refactor: move atomicRearrange into Zcf

Chris-Hibbert mentioned this pull request May 31, 2024

chore: Integrate on top of upgrade-15 #9436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move atomicRearrange into Zcf #7900

refactor: move atomicRearrange into Zcf #7900

Chris-Hibbert commented Jun 8, 2023 •

edited

Loading

Chris-Hibbert commented Jun 8, 2023

turadg Jun 9, 2023

Chris-Hibbert Jun 9, 2023

turadg Jun 9, 2023 •

edited

Loading

Chris-Hibbert Jun 23, 2023

erights Jun 23, 2023

Chris-Hibbert Jun 23, 2023

turadg Jun 23, 2023

Chris-Hibbert Jun 23, 2023

turadg Jun 23, 2023

Chris-Hibbert Jun 23, 2023

turadg Jun 23, 2023

Chris-Hibbert Jun 23, 2023

turadg Jul 21, 2023 •

edited

Loading

Chris-Hibbert Jul 21, 2023

erights commented Jul 16, 2023 •

edited

Loading

turadg left a comment

turadg left a comment

	*/
	*/
	export const atomicRearrange = (zcf, transfers) => zcf.atomicRearrange(transfers);

refactor: move atomicRearrange into Zcf #7900

refactor: move atomicRearrange into Zcf #7900

Conversation

Chris-Hibbert commented Jun 8, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Release considerations

Chris-Hibbert commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights commented Jul 16, 2023 • edited Loading

turadg left a comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Chris-Hibbert commented Jun 8, 2023 •

edited

Loading

turadg Jun 9, 2023 •

edited

Loading

turadg Jul 21, 2023 •

edited

Loading

erights commented Jul 16, 2023 •

edited

Loading