Simplify Pegasus for POLA #3986

michaelfig · 2021-10-19T21:43:12Z

Description

Rather than exposing publicFacet information about the current pegs, use local actions objects to get things done. These are exposed to the connection owner, and return information about the connection via Subscriptions.

Security Considerations

Should improve auditability via POLA.

Documentation Considerations

The bootstrap Pegasus now puts its powerful authorities in agoricNames.lookup('pegasusConnections'), which is only given out to powerful provisioned solos.

Nobody was relying on this information before, so there should be no loss of functionality.

Testing Considerations

More tests are done in packages/pegasus.

katelynsills · 2021-10-22T20:30:59Z

This looks like a great simplification. For me to fully understand, I could probably use a walk through on Zoom.

Also, if you have time, could you point me to the latest IBC docs that would be helpful for me to review to understand the requirements? I can also try to take a look myself for the latest and just start at the beginning.

michaelfig · 2021-10-23T01:17:58Z

For me to fully understand, I could probably use a walk through on Zoom.

Yeah, let's find some time next week.

Also, if you have time, could you point me to the latest IBC docs that would be helpful for me to review to understand the requirements?

The ICS20 transfer protocol we're trying to implement is something like: https://github.com/cosmos/ibc/blob/master/spec/app/ics-020-fungible-token-transfer/README.md

We use the Agoric network API as our interface to IBC (and potentially other transports), so it abstracts most of the details.

erights · 2021-11-15T05:39:31Z

For me to fully understand, I could probably use a walk through on Zoom.

Yeah, let's find some time next week.

Did this meeting happen? Was it recorded? If it did not happen, might it happen this week? If so, I'd like to be there.

What's the status of this PR? The notifier type reform at least seems to have been taken care of in other PRs.

erights

Just shallow comments so far. Still getting my feet wet.

packages/pegasus/src/once-promise-kit.js

packages/pegasus/src/courier.js

packages/pegasus/src/once-promise-kit.js

packages/pegasus/src/pegasus.js

erights · 2021-12-13T09:25:13Z

packages/pegasus/src/pegasus.js

+    localDenomState,
+    transferProtocol,
+  }) => {
+    let checkAbort = () => {};


Nice little pattern

packages/pegasus/src/pegasus.js

dckc

I can't say that I understand this code well enough to maintain it now, but I can see that this PR is a clear improvement. I have a few non-critical comments / suggestions.

At least the happy paths are covered in the unit test, and I expect this will get considerable integration testing shortly.

Let's go for it.

dckc · 2022-02-04T20:25:07Z

packages/pegasus/src/ics20.js

+import { assert, details as X } from '@agoric/assert';
+
+/**
+ * @typedef {Object} ICS20TransferPacket


cite the (relevant part of the) ICS-20 spec here, pls?

dckc · 2022-02-04T20:30:49Z

packages/pegasus/src/ics20.js

+ * @returns {Promise<PacketParts>}
+ */
+export const parseICS20TransferPacket = async packet => {
+  /** @type {ICS20TransferPacket} */


I don't think we've established this type just by doing a JSON.parse.

Suggested change

/** @type {ICS20TransferPacket} */

/** @type {unknown} */

dckc · 2022-02-04T20:34:39Z

packages/pegasus/src/ics20.js

+}) => {
+  // We're using Nat as a dynamic check in a way that tsc doesn't grok.
+  // Should Nat's parameter type be `unknown`?
+  // @ts-ignore


@ts-expect-error?

Document limitation of value to fungible tokens?

dckc · 2022-02-04T20:34:59Z

packages/pegasus/src/ics20.js

+  depositAddress,
+}) => {
+  // We're using Nat as a dynamic check in a way that tsc doesn't grok.
+  // Should Nat's parameter type be `unknown`?


yes, it should. Bonus points for raising an issue.

dckc · 2022-02-04T20:38:59Z

packages/pegasus/src/ics20.js

+ * @returns {Promise<void>}
+ */
+export const assertICS20TransferPacketAck = async ack => {
+  const { success, error } = JSON.parse(ack);


This could crash (bad JSON, not an object). As discussed, we accept stack traces as a diagnostic here.

dckc · 2022-02-04T21:11:31Z

packages/pegasus/src/pegasus.js

+          winnerKeyword,
+          winner,
+        ) => {
+          // Transfer the amount to our backing seat.


our backing seat... that's poolSeat, right? I don't see any reference to it in this function. Is poolSeat is always the winner? If so, why pass it in?

I think I see what's going on now...

Suggested change

// Transfer the amount to our backing seat.

// Transfer the amount to / from our backing seat.

dckc · 2022-02-04T21:14:34Z

packages/pegasus/src/pegasus.js

       */
-      const denomUriToCourier = makeStore('Denomination');
+      // Legacy because the value contains a JS Set


Bonus points for citing an issue that represents plans to clean this up.

michaelfig requested review from erights and katelynsills October 19, 2021 21:43

michaelfig self-assigned this Oct 19, 2021

michaelfig added notifier pegasus Core Economy OBSOLETE in favor of INTER-protocol labels Oct 19, 2021

michaelfig force-pushed the mfig-pegasus-simplify branch from b8b783d to 7a96db5 Compare November 5, 2021 18:56

michaelfig force-pushed the mfig-pegasus-simplify branch from 7a96db5 to 36b5c84 Compare November 15, 2021 19:18

michaelfig force-pushed the mfig-pegasus-simplify branch from 36b5c84 to 03d4cc5 Compare November 22, 2021 23:35

michaelfig removed the request for review from katelynsills November 22, 2021 23:35

michaelfig force-pushed the mfig-pegasus-simplify branch from 03d4cc5 to b84c30d Compare November 25, 2021 20:18

michaelfig force-pushed the mfig-pegasus-simplify branch 2 times, most recently from c915758 to 0ebb7be Compare December 4, 2021 18:35

dckc mentioned this pull request Dec 6, 2021

factor out makePeg / retain / redeem in pegasus contract? #4152

Open

erights reviewed Dec 13, 2021

View reviewed changes

michaelfig force-pushed the mfig-pegasus-simplify branch from 0ebb7be to 16f6b17 Compare December 15, 2021 18:39

erights self-requested a review December 16, 2021 19:09

michaelfig mentioned this pull request Jan 16, 2022

Pegasus rollout #4306

Closed

6 tasks

michaelfig added 9 commits February 4, 2022 14:17

feat(pegasus)!: make more robust and remove public state

a5413f7

feat(pegasus): rejectStuckTransfers, getRemoteDenomSubscription

54bf0bc

fix: rejectStuckTransfers sends a TransferProtocol-level error

8374f94

fix(meter): use process.env not process.environment

c703d07

fix(pegasus): more POLA and less global state

e7ea320

feat(pegasus): properly abort on connection close

1b17f7a

docs(pegasus): update demo.md

7ca2ffb

fix(notifier): make the Updater a Far object

20707a1

refactor(pegasus): code cleanups

87aeed6

michaelfig force-pushed the mfig-pegasus-simplify branch from 16f6b17 to 9b072fe Compare February 4, 2022 20:21

dckc approved these changes Feb 4, 2022

View reviewed changes

michaelfig added 4 commits February 4, 2022 15:56

style(pegasus): reformat prettier

c77c582

fix(pegasus): more robust ics20 JSON parsing

208109f

fix(pegasus): use bigints for nonces

9065d6e

style: reformat with prettier

77ebadb

michaelfig force-pushed the mfig-pegasus-simplify branch from 23df041 to 77ebadb Compare February 4, 2022 21:56

michaelfig added the automerge:no-update (expert!) Automatically merge without updates label Feb 4, 2022

Merge branch 'master' into mfig-pegasus-simplify

bb90e44

mergify bot merged commit 529bf23 into master Feb 4, 2022

mergify bot deleted the mfig-pegasus-simplify branch February 4, 2022 22:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Pegasus for POLA #3986

Simplify Pegasus for POLA #3986

michaelfig commented Oct 19, 2021 •

edited

Loading

katelynsills commented Oct 22, 2021

michaelfig commented Oct 23, 2021

erights commented Nov 15, 2021

erights left a comment

erights Dec 13, 2021

dckc left a comment

dckc Feb 4, 2022

dckc Feb 4, 2022

dckc Feb 4, 2022

dckc Feb 4, 2022

dckc Feb 4, 2022

dckc Feb 4, 2022

dckc Feb 4, 2022

dckc Feb 4, 2022

dckc Feb 4, 2022

	// Transfer the amount to our backing seat.
	// Transfer the amount to / from our backing seat.

Simplify Pegasus for POLA #3986

Simplify Pegasus for POLA #3986

Conversation

michaelfig commented Oct 19, 2021 • edited Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

katelynsills commented Oct 22, 2021

michaelfig commented Oct 23, 2021

erights commented Nov 15, 2021

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc left a 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

Choose a reason for hiding this comment

michaelfig commented Oct 19, 2021 •

edited

Loading