fix(amm): Prevent creation of constant product AMM with non-fungible central token #4476
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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. |
dckc
added
automerge:rebase
|
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.NATSecurity Considerations
n/a
Documentation Considerations
Testing Considerations
Includes test that demonstrates failure to create an AMM with a AssetKind.SET