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

await scaledPriceAuthority #8452

Merged
merged 4 commits into from
Oct 12, 2023
Merged

await scaledPriceAuthority #8452

merged 4 commits into from
Oct 12, 2023

Conversation

turadg
Copy link
Member

@turadg turadg commented Oct 12, 2023

Description

In devnet-21 we observed an error,

Error: auction observer of [object Alleged: stATOM brand] failed: Error: key (an object) not found in collection "brandIn"

It hasn't been reproduced but it's due to a race condition that we should prevent. This prevents it by providing a signal as to when the scaledPriceAuthority (in the real stATOM brand) is available, and awaits that before calling code that may use it.

Security Considerations

Publishing the instance in agoricName. This is okay just as the authority feeds are there.

Scaling Considerations

--

Documentation Considerations

  • include more documentation about the design of these feeds.

Testing Considerations

Forgoing a test that induces the race condition but this code will be tested. The publishing is tested by the await.

Upgrade Considerations

Not on-chain code.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing call to scaledPriceFeedName seems important

@@ -12,8 +12,11 @@ import { Stable } from '@agoric/internal/src/tokens.js';
import { TimeMath } from '@agoric/time/src/timeMath.js';
import { makePromiseKit } from '@endo/promise-kit';

import { instanceNameFor } from './price-feed-proposal.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay for reducing coupling here!

These go into separate bundles, so including price-feed in addAssetToVault was awkward. This should reduce bundle sizes.

@@ -135,6 +138,7 @@ export const registerScaledPriceAuthority = async (
priceAuthorityAdmin,
priceAuthority,
},
instance: { produce: produceInstance },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment / security concern: small increase in authority - authority to produce/replace any agoricNames.instance. But since we can't know issuerName in advance, this is the least authority we can grant within the expressiveness of our permits.

@@ -211,9 +215,11 @@ export const registerScaledPriceAuthority = async (
}),
);

const label = `scaledPriceAuthority-${issuerName}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scaledPriceFeedName is nicely factored out but... oops! not used here.

though not a correctness issue, important for maintenance / documentation

// don't add the collateral offering to vaultFactory until its price feed is available
// eslint-disable-next-line no-restricted-syntax -- allow this computed property
await consumeInstance[oracleInstanceName];
await consumeInstance[oracleBrandFeedName(oracleBrand, 'USD')];
// await also the real brand
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: editorial:

Suggested change
// await also the real brand
// await also the negotiable brand

@@ -420,8 +433,8 @@ export const getManifestForAddAssetToVault = (
priceAuthorityAdmin: true,
priceAuthority: true,
},
produce: {
scaledPriceAuthorityKits: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vestigial. nice clean-up

@turadg turadg force-pushed the 72-await-scaledPriceAuthority branch from 180c78b to 579fbf1 Compare October 12, 2023 16:06

The intended flow is that:
1. a negotiable brand is created (e.g. ATOM)
2. a price provider says “i can give you quotes for that” and runs price-feed-proposal. That makes “oracleBrands” (which are inert and have a separate identity so that they don’t have the authority to say they’re the real quote for it).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume 'inert' means no payments or purses. If that's the case, please say so explicitly. Is there any way for holders of the issuer or the brand to reliably detect that it is inert?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, there's no mint or issuer. But there's no way to inspect the brand to find out.

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a correctness issue, I guess

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Oct 12, 2023
@mergify mergify bot merged commit 13a758d into master Oct 12, 2023
80 checks passed
@mergify mergify bot deleted the 72-await-scaledPriceAuthority branch October 12, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants