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

feat: advancer integrates with LP and contract #10518

Merged

mergify merged 4 commits into master from pc/fusdc-advancer-integrates-with-lp

Nov 20, 2024

Member

0xpatrickdev commented Nov 18, 2024 •

edited

Loading

closes: #10390

Description

Updates Advancer to use LiquidityPool borrower facet
- previous iteration used payments instead of seats
Integrates Advancer with contract.js
- creates a PoolAccount using a flow to provide to makeAdvancer

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Includes exo tests for Advancer. Also includes a contract test covering the happy path, although out of scope for the ticket.

Upgrade Considerations

None, unreleased

0xpatrickdev force-pushed the pc/fusdc-advancer-with-lp branch 6 times, most recently from a78e88c to 3117eef Compare

November 19, 2024 20:16

Base automatically changed from pc/fusdc-advancer-with-lp to master

November 19, 2024 21:00

0xpatrickdev force-pushed the pc/fusdc-advancer-integrates-with-lp branch from 9ee81ea to b4a6de7 Compare

November 19, 2024 22:08

cloudflare-workers-and-pages bot commented Nov 19, 2024 •

edited

Loading

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`7ec6fcf`
Status:	✅ Deploy successful!
Preview URL:	https://5f431db8.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-fusdc-advancer-integrates.agoric-sdk.pages.dev

0xpatrickdev force-pushed the pc/fusdc-advancer-integrates-with-lp branch 2 times, most recently from 85ebd94 to 2e699e7 Compare

November 20, 2024 00:18

0xpatrickdev marked this pull request as ready for review

November 20, 2024 00:18

0xpatrickdev requested a review from a team as a code owner

November 20, 2024 00:18

0xpatrickdev requested review from AgoricTriage, turadg and dckc

November 20, 2024 00:18

0xpatrickdev commented

View reviewed changes

packages/fast-usdc/test/fast-usdc.contract.test.ts Outdated Show resolved Hide resolved

0xpatrickdev commented

View reviewed changes

packages/fast-usdc/src/fast-usdc.contract.js Show resolved Hide resolved

dckc reviewed

View reviewed changes

Member

dckc left a comment

I don't see anything wrong.

I think I skimmed some parts, though. I wonder if I should give it another look before approving.

packages/fast-usdc/src/exos/advancer.js Outdated

-               *   borrow(amount: Amount<"nat">): Promise<Payment<"nat">>;
-               *   repay(payments: PaymentKeywordRecord): Promise<void>
-               * }} AssetManagerFacet
+               * @import {BorrowerFacet} from './liquidity-pool.js';

Member

dckc Nov 20, 2024

hm... do we want to name the types of each of the facets? I can see arguments both ways, but consider...

Suggested change

      
             * @import {BorrowerFacet} from './liquidity-pool.js';
          
             * @import {LiquidityPoolKit['borrower']} from './liquidity-pool.js';

packages/fast-usdc/src/exos/advancer.js

+                  statusManager,
+                  usdc,
+                  vowTools: { watch, when },
+                  zcf,

Member

dckc Nov 20, 2024

I only see zcf.makeEmptySeatKit used. Narrow this to that 1 method?

Member Author

0xpatrickdev Nov 20, 2024

Agree this would be helpful towards limiting authority, but I don't think we can do this without making a new exo with something like prepareGuardedAttenuator or the like.

Would you still like to see this in this PR? I also suspect Settler will need the same - I can add a TODO if you're planning to tackle this there?

Member

dckc Nov 20, 2024

I don't think we can do this without making a new exo ...

Right. Never mind.

packages/fast-usdc/src/exos/advancer.js Outdated

+                  usdc,
+                  vowTools: { watch, when },
+                  zcf,
+                  zoeTools,

Member

dckc Nov 20, 2024

likewise, narrow to localTransfer?

packages/fast-usdc/src/exos/advancer.js

Comment on lines 138 to 140

                             log(
                               `Insufficient pool funds`,
                               `Requested ${q(advanceAmount)} but only have ${q(poolBalance)}`,

Member

dckc Nov 20, 2024

note: I wonder if this makes use of the ses console as it should.

There's nothing to censor here. But "a more powerful distributed causal console" could be really handy.

Not for this PR, but I wonder when.

packages/fast-usdc/src/exos/advancer.js

Comment on lines -138 to -139

		// report `requestedAmount`, not `advancedAmount`... do we need to
		// communicate net to `StatusManger` in case fees change in between?

Member

dckc Nov 20, 2024

That's an interesting idea: compute the split at advancement time.

Member Author

0xpatrickdev Nov 20, 2024

Yes, something to keep in our back packet. When .settle is called, it could return the split amounts. But it's still not entirely clear to me if we should wait to .settle() until the payments have actually returned back to the LP, or settling on intent is acceptable.

packages/fast-usdc/src/exos/advancer.js Outdated

+                            const amountKWR = harden({ USDC: advanceAmount });
+                            borrowerFacet.borrow(tmpSeat, amountKWR);
+                            const depositV = zoeTools.localTransfer(

Member

dckc Nov 20, 2024

the comment below says that depositHandler handles failures in localTransfer, so it seems odd to have this inside the try

I suppose we can think again when we do #10510 (comprehensive error testing) but I struggle to be confident that we will.

Member Author

0xpatrickdev Nov 20, 2024

Good feedback, revis(ed|ing) to only include .borrow() in the try.

Regarding ".borrow() might fail if the balance changes since we requested it" - I wonder if this comment can be removed since we changed the implementation to use seats. Since there are no awaits in between .getBalance() and .borrow(), and we have await null at the beginning of handleTransactionEvent, can we safely assume we'll be in the same turn?

Member

dckc Nov 20, 2024

we safely assume we'll be in the same turn?

yes, it's statically evident that there are no turn boundaries between .getBalance() and the return from .borrow().

packages/fast-usdc/src/exos/advancer.js

Comment on lines +207 to +209

+                        // TODO return seat allocation from ctx to LP?
                         log('🚨 advance deposit failed', q(error).toString());
-                        log('TODO live payment to return to LP', q(payment).toString());
+                        // TODO #10510 (comprehensive error testing) determine

Member

dckc Nov 20, 2024

this stuff is why #10510 looks to me like "the other 80% of the work".

The local deposit would only fail due to things in our control at design-time (i.e. bugs), but the remote transfer can fail for runtime reasons (timeout).

Member Author

0xpatrickdev Nov 20, 2024

The local deposit would only fail due to things in our control at design-time (i.e. bugs), but the remote transfer can fail for runtime reasons (timeout).

The is well-put - I should include this verbiage as a code comment.

Agree we can almost guarantee .deposit() will never fail, so I decided not to add more noise to this PR with a repay() method on the borrower facet. Also agree .transfer() failing is the more important failure path to be concerned with.

packages/fast-usdc/src/fast-usdc.contract.js Show resolved Hide resolved

packages/fast-usdc/test/exos/advancer.test.ts Outdated

                 t.deepEqual(inspectLogs(0), [
                   'Advance transfer fulfilled',
-                  '{"amount":{"brand":"[Alleged: USDC brand]","value":"[150000000n]"},"destination":{"chainId":"osmosis-1","encoding":"bech32","value":"osmo183dejcnmkka5dzcu9xw6mywq0p2m5peks28men"},"result":"[undefined]"}',
+                  '{"amount":{"brand":"[Alleged: USDC brand]","value":"[149999899n]"},"destination":{"chainId":"osmosis-1","encoding":"bech32","value":"osmo183dejcnmkka5dzcu9xw6mywq0p2m5peks28men"},"result":"[undefined]"}',

Member

dckc Nov 20, 2024

suggestion: add a test to fees.tests.ts using the numbers in this exo test such as 149999899n.

Then I wouldn't have to do arithmetic to review this PR.

Member Author

0xpatrickdev Nov 20, 2024

Good suggestion. See ac61b3666e

Member Author

0xpatrickdev Nov 20, 2024

Perhaps better without the underscores. LMK

0xpatrickdev force-pushed the pc/fusdc-advancer-integrates-with-lp branch from 2e699e7 to 6e3eba4 Compare

November 20, 2024 03:40

turadg approved these changes

View reviewed changes

Member

turadg left a comment

LGTM. We'll need more contract and error coverage but those are scheduled separately.

packages/fast-usdc/src/fast-usdc.contract.js

                 const creatorFacet = zone.exo('Fast USDC Creator', undefined, {
                   /** @type {(operatorId: string) => Promise<Invitation<OperatorKit>>} */
                   async makeOperatorInvitation(operatorId) {
+                    // eslint-disable-next-line no-use-before-define

Member

turadg Nov 20, 2024

I didn't realize that #9361 hadn't landed. I'll try to get that in now.

packages/fast-usdc/src/fast-usdc.flows.js

+               * @satisfies {OrchestrationFlow}
+               * @param {Orchestrator} orch
+               */
+              export const makeLocalAccount = async orch => {

Member

turadg Nov 20, 2024

is this our first flow? I thought we would be able to ship this without async-flow.

This is fine since it runs once but I'm curious what it would take to do this without a flow.

Member Author

0xpatrickdev Nov 20, 2024

curious what it would take to do this without a flow

Less complicated than a CosmosOrchAccount Maybe something like:

const makeLocalOrchestrationAccountKit = prepareLocalOrchestrationAccountKit(
  zone,
  {
    makeRecorderKit,
    zcf,
    timerService,
    vowTools,
    chainHub,
    localchain,
    zoeTools,
  },
);

const account = await when(E(localchain).makeAccount());
const address = await when(E(account).getAddress());

const { holder } = makeLocalOrchestrationAccountKit({
  account,
  address: harden({
    value: address,
    encoding: 'bech32',
    chainId: localChainInfo.chainId,
  }),
  // FIXME storage path https://github.com/Agoric/agoric-sdk/issues/9066
  storageNode: childNode,
});

Something we can revisit . Also - I'm reminded that #9066 might need some love as we are getting to launch this FUSDC.


          test: FeeTools against MockCctpTxEvidences

d5aeada

- document `MockCctpTxEvidences` with `FeeConfig` used `commonPrivateArgs.feeConfig` (contract tests)
- increase `makeTestFeeConfig` `maxVariable` value so fees are more visible in tests
- add `debugString` to `feeToolsScenario` macro for easier maintenance

0xpatrickdev force-pushed the pc/fusdc-advancer-integrates-with-lp branch from 6e3eba4 to 63d7b44 Compare

November 20, 2024 15:35

0xpatrickdev added 3 commits

November 20, 2024 11:25


          feat: Advancer uses borrower facet

35eb7ad


          feat: integrate Advancer with contract

c5d67af


          test: Advancer contract test

7ec6fcf

0xpatrickdev force-pushed the pc/fusdc-advancer-integrates-with-lp branch from 63d7b44 to 7ec6fcf Compare

November 20, 2024 16:25

0xpatrickdev added the automerge:rebase label

mergify bot merged commit 8c79835 into master

91 checks passed

mergify bot deleted the pc/fusdc-advancer-integrates-with-lp branch

November 20, 2024 17:07

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

Labels

automerge:rebase