Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
326 stars 205 forks source link

AMM UI hung due to hung / dead issuer #4512

Open dckc opened 2 years ago

dckc commented 2 years ago

Describe the bug

The devnet AMM is hung, most likely because I added a pool based on an off-chain issuer and then exited my client, hence taking down the issuer.

To Reproduce

This is what I did, anyway. I haven't tried to do it again.

  1. in a deploy script, use makeIssuerKit and put it on the board (and put the mint in home.scratch) and leave the deploy script awaiting a never-resolving promise.
  2. E(ammAPI).addPool(issuer, 'Note2') and add liquidity. See the Note2 issuer in the AMM (screenshot below)
  3. Kill the deploy script.
  4. Run it again, creating a Cat token.
  5. Add Cat to the AMM. Try to look at it in the AMM.
  6. The AMM is hung. (screenshot below)

full REPL log: https://gist.github.com/dckc/25c4f28f039c08f7cf1c460196396ac9

Expected behavior

AMM handles Cat tokens like Note2 tokens.

Platform Environment

devnet and the docker wallet

Additional context

This is the deploy script that was originally used for Note and then editing for Cat:

import { E } from '@endo/far';
import { deeplyFulfilled } from '@endo/marshal';

import { makeIssuerKit, AssetKind } from '@agoric/ertp';

/**
 * @param {Promise<{zoe: ERef<ZoeService>, board: ERef<Board>, agoricNames:
 * Object, wallet: ERef<Object>, faucet: ERef<Object>}>} homePromise
 */
const deployContract = async (homePromise) => {
  console.log('awaiting home promise...');
  const home = await homePromise;

  const kit = makeIssuerKit('Cat', AssetKind.NAT, harden({ decimalPlaces: 2 }));
  console.log('publishing issuer, brand to board');
  const published = await deeplyFulfilled(
    harden({
      proposedName: 'Note',
      issuer: E(home.board).getId(kit.issuer),
      brand: E(home.board).getId(kit.brand),
    }),
  );
  console.log(published);
  console.log('saving noteMint to personal scratchpad...');
  await E(home.scratch).set('noteMint', kit.mint);

  // Keep the issuer etc. around for the duration of the demo
  await new Promise(() => {});
};

export default deployContract;

Screenshots

It worked the 1st time with Note2:

paste (1)

But with Cat, it hangs:

image

cc @michaelfig

samsiegart commented 2 years ago

This may take a bit of effort for me to repro, do you happen to have the browser console output, any errors?

samsiegart commented 2 years ago

Seems like this is specifically caused by killing the deploy script, as opposed to adding the cat token. It's not able to load the brand anymore because it disappeared with the script, and the app hangs because the setup fails. I'm not sure the extent to which we want to engineer around this, since it seems specific to a local setup. A couple ideas:

I'm leaning towards the latter because if it can't load an issuer from the chain then that's probably a sign that something went very wrong. However, that wouldn't fix the expected behavior for this specific local setup.

michaelfig commented 2 years ago

I'm leaning towards the latter because if it can't load an issuer from the chain then that's probably a sign that something went very wrong. However, that wouldn't fix the expected behavior for this specific local setup.

When the deploy script is killed, it's the same as if an issuer returns promise rejection from an E(...). We don't want to. trust an issuer, so should make sure that every E(...) can tolerate failure, either by being awaited, or by putting a .catch that returns a default value.

samsiegart commented 2 years ago

When the deploy script is killed, it's the same as if an issuer returns promise rejection from an E(...)

I was assuming that wouldn't be the case normally, but I suppose a non-standard issuer could reject for all sorts of reasons? That makes a bit more sense

dckc commented 2 years ago

See also Agoric/documentation#663 .

Tartuffo commented 2 years ago

@samsiegart Talk to @dtribble about this from an architecture perspective. Timeouts == bad.

Tartuffo commented 2 years ago

In order to trigger this, you have to fire up an Ag Solo and make an issuer off chain and turn off Ag Solo. Taking this out of MN-1.

erights commented 2 years ago

I'm leaning towards the latter because if it can't load an issuer from the chain then that's probably a sign that something went very wrong. However, that wouldn't fix the expected behavior for this specific local setup.

When the deploy script is killed, it's the same as if an issuer returns promise rejection from an E(...). We don't want to. trust an issuer, so should make sure that every E(...) can tolerate failure, either by being awaited, or by putting a .catch that returns a default value.

A misbehaving issuer may also never respond, leaving all promise-for-results-of E calls unresolved. Without resorting to timeouts, this should not block other operations. Do we know what code is making which E call whose lack of a successful response (whether error or just unresponsive) is causing things to hang?