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

validate allocation in getCurrentAllocation #7198

Open
turadg opened this issue Mar 21, 2023 · 1 comment
Open

validate allocation in getCurrentAllocation #7198

turadg opened this issue Mar 21, 2023 · 1 comment
Labels
code-style defensive correctness patterns; readability thru consistency devex developer experience enhancement New feature or request vaults_triage DO NOT USE

Comments

@turadg
Copy link
Member

turadg commented Mar 21, 2023

What is the Problem Being Solved?

We had a bug where the record from getCurrentAllocation was missing a key that was expected. This is because the return type is Record<Keyword, Amount<any>: #7195 (review)

Description of the Design

Make getCurrentAllocation return an accurate type description:

  • the key may not be present in the record
  • the amount kind may not be as expected

I tried making it return an UnknownKeywordRecord,

/**
 * @typedef {Record<Keyword, Amount<any>>} AmountKeywordRecord
 *
 * The keys are keywords, and the values are amounts. For example:
 * { Asset: AmountMath.make(assetBrand, 5n), Price:
 * AmountMath.make(priceBrand, 9n) }
 *
 * NB: this type hides the fact that the key may be absent or the
 * the amount may be of a different kind.
 * @see {UnknownAmountKeywordRecord}
 */

/**
 * @typedef {Record<Keyword, Amount | undefined>} UnknownAmountKeywordRecord
 */

That causes errors in the many places the allocation structure is assumed. We could have runtime checks in all those places but I propose that we instead add an optional "shape" parameter to getCurrentAllocation. It can mustMatch before returning. The return type should be aware of this runtime check. Making that work will depend on being able to infer the shape's static type from its own structure: #6160

Security Considerations

Scaling Considerations

Test Plan

@turadg turadg added enhancement New feature or request code-style defensive correctness patterns; readability thru consistency devex developer experience labels Mar 21, 2023
@erights
Copy link
Member

erights commented Mar 22, 2023

I like the idea of adding an optional shape parameter that gets mustMatched if present. Nice!

@ivanlei ivanlei added the vaults_triage DO NOT USE label Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-style defensive correctness patterns; readability thru consistency devex developer experience enhancement New feature or request vaults_triage DO NOT USE
Projects
None yet
Development

No branches or pull requests

3 participants