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.

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

AMM UI hung due to hung / dead issuer #4512

Open
dckc opened this issue Feb 9, 2022 · 8 comments
Open

AMM UI hung due to hung / dead issuer #4512

dckc opened this issue Feb 9, 2022 · 8 comments
Assignees
Labels
AMM bug Something isn't working Core Economy OBSOLETE in favor of INTER-protocol security UI

Comments

@dckc
Copy link
Member

dckc commented Feb 9, 2022

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

@dckc dckc added bug Something isn't working UI Core Economy OBSOLETE in favor of INTER-protocol AMM labels Feb 9, 2022
@samsiegart
Copy link
Contributor

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

@samsiegart
Copy link
Contributor

samsiegart commented Feb 9, 2022

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:

  • Isolate errors from specific issuers better, don't make the app hang, just show the issuers that successfully load
  • Let the app fail to load and just show an error message instead of a perpetual loading indicator, maybe with a retry/refresh button.

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

dckc commented Feb 9, 2022

See also Agoric/documentation#663 .

@Tartuffo Tartuffo added the restival to be done before RUN Protocol Purple Team festival label Feb 11, 2022
@Tartuffo
Copy link
Contributor

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

@Tartuffo
Copy link
Contributor

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.

@Tartuffo Tartuffo removed this from the Mainnet 1 RC0 milestone Aug 10, 2022
@erights
Copy link
Member

erights commented Sep 6, 2022

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?

@ivanlei ivanlei removed the restival to be done before RUN Protocol Purple Team festival label Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AMM bug Something isn't working Core Economy OBSOLETE in favor of INTER-protocol security UI
Projects
None yet
Development

No branches or pull requests

6 participants