-
Notifications
You must be signed in to change notification settings - Fork 206
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
fix(amm): Prevent creation of constant product AMM with non-fungible central token #4476
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using assert.equal
and including both sides in the failure diagnostic.
I'd like to hear from @Chris-Hibbert on clear code that costs an await
round trip vs. a Promise.all
optimization.
packages/run-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js
Outdated
Show resolved
Hide resolved
packages/run-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js
Outdated
Show resolved
Hide resolved
@@ -115,6 +116,13 @@ const start = async (zcf, privateArgs) => { | |||
} = /** @type { Terms & AMMTerms } */ (zcf.getTerms()); | |||
assertIssuerKeywords(zcf, ['Central']); | |||
assert(centralBrand !== undefined, X`centralBrand must be present`); | |||
|
|||
const centralDisplayInfo = await E(centralBrand).getDisplayInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This await
adds a round-trip to contract startup. That could be avoided by combining it using Promise.all
with const paramManager = await makeParamManager(
below, at the cost of some code clarity.
@Chris-Hibbert I'm curious what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My usual rule of thumb is to not worry about performance issues (particularly one-time start up costs) until measurement shows them to be a problem, but I think you've been actually measuring start up times, so anyway.
I don't have a problem with using Promise.all()
to aggregate awaits even when they're working on different tasks. This test doesn't have to be the first thing done. It's entire purpose would be to prevent accidentally breaking the AMM, so it should never fail if the AMM would have been useful.
@@ -115,6 +116,13 @@ const start = async (zcf, privateArgs) => { | |||
} = /** @type { Terms & AMMTerms } */ (zcf.getTerms()); | |||
assertIssuerKeywords(zcf, ['Central']); | |||
assert(centralBrand !== undefined, X`centralBrand must be present`); | |||
|
|||
const centralDisplayInfo = await E(centralBrand).getDisplayInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a risk that centralBrand
will tell us it's fungible here but then behave as non-fungible later. I suppose behaving as non-fungible later is already protected against, and the point of this check is to get a clear and relevant diagnostic rather than "array found where bigint expected" or some such later. So... never mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it says it uses AssetKind.NAT`, then it's giving us permission to use the corresponding AmountMath. If it misbehaves after that, it's a broken issuer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right... it's just that I'm new at evaluating the risks around broken issuers, so I was thinking out loud.
Co-authored-by: Dan Connolly <dckc@madmode.com>
Co-authored-by: Dan Connolly <dckc@madmode.com>
packages/run-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js
Outdated
Show resolved
Hide resolved
@@ -115,6 +116,13 @@ const start = async (zcf, privateArgs) => { | |||
} = /** @type { Terms & AMMTerms } */ (zcf.getTerms()); | |||
assertIssuerKeywords(zcf, ['Central']); | |||
assert(centralBrand !== undefined, X`centralBrand must be present`); | |||
|
|||
const centralDisplayInfo = await E(centralBrand).getDisplayInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My usual rule of thumb is to not worry about performance issues (particularly one-time start up costs) until measurement shows them to be a problem, but I think you've been actually measuring start up times, so anyway.
I don't have a problem with using Promise.all()
to aggregate awaits even when they're working on different tasks. This test doesn't have to be the first thing done. It's entire purpose would be to prevent accidentally breaking the AMM, so it should never fail if the AMM would have been useful.
@@ -115,6 +116,13 @@ const start = async (zcf, privateArgs) => { | |||
} = /** @type { Terms & AMMTerms } */ (zcf.getTerms()); | |||
assertIssuerKeywords(zcf, ['Central']); | |||
assert(centralBrand !== undefined, X`centralBrand must be present`); | |||
|
|||
const centralDisplayInfo = await E(centralBrand).getDisplayInfo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it says it uses AssetKind.NAT`, then it's giving us permission to use the corresponding AmountMath. If it misbehaves after that, it's a broken issuer.
packages/run-protocol/src/vpool-xyk-amm/multipoolMarketMaker.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Dan Connolly <dckc@madmode.com>
@Chris-Hibbert note that @b4d2 doesn't have merge rights. When this is ready, I guess it's up to you to merge it. (Let me know if you would rather I do it.) @b4d2 we're used to having the author merges their own work, so this may have fallen between the cracks. |
@dckc do you have time to fix the conventional commit issues and submit this? I'm happy with the current state. |
squashing addressed the conventional-commits stuff. (they don't block a merge in any case) |
Thanks for the contribution, @b4d2! |
Ah! I thought they did. Thanks! |
closes: #4047
Description
Adds validation of AssetKind at AMM creation to ensure Central is of
AssetKind.NAT
Security Considerations
n/a
Documentation Considerations
Testing Considerations
Includes test that demonstrates failure to create an AMM with a AssetKind.SET