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

9220 deprecate getDisplayInfo #9221

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Conversation

turadg
Copy link
Member

@turadg turadg commented Apr 10, 2024

closes: #9220

Description

Deprecate getDisplayInfo so we don't use it more.
Point TODOs to removing it: #10235
Removes the use in auctionBook by using a fixed scaling factor

Also converts the JSDoc types to .ts so @deprecated can be used

Security Considerations

None yet but #10235 will change the source of decimal places

Scaling Considerations

none

Documentation Considerations

https://docs.agoric.com/reference/ertp-api/brand.html#abrand-getdisplayinfo will need updating.

Testing Considerations

Upgrade Considerations

packages/smart-wallet/src/smartWallet.js Outdated Show resolved Hide resolved
packages/smart-wallet/src/walletFactory.js Outdated Show resolved Hide resolved
Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

I'm in favor of dropping decimalPlaces from Brands.

packages/ERTP/src/types.d.ts Outdated Show resolved Hide resolved
@@ -470,6 +470,7 @@ export const prepareAuctionBook = (baggage, zcf, makeRecorderKit) => {
trace('observing');

void E.when(
// FIXME can this work without decimalPlaces? if so, read from boardAux
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't need decimalPlaces, but would need some scaling factor to replace it. It could use BASIS_POINTS or MILLIONS instead. It just needs something so the resolution of the response is at least nominal. Asking for price quotes on Satoshis or smallest fraction of BLD wouldn't be workable.

Copy link
Member Author

Choose a reason for hiding this comment

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

why does the code look up getDisplayInfo if a nominal scaling factor suffices? Could this just skip the lookup and use DEFAULT_DECIMALS?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a nominal factor would suffice. When this was built, I didn't think deeply enough to be sure that a nominal factor would be fine, and we had getDisplayInfo, much as I would have preferred that we didn't.

@michaelfig seems to have pointed to some places where he wanted to know what the actual "unit amounts" were. (i.e. "what is the unit of currency?") But for the auction's goals, it just needs to ensure it's not asking for pricing on an amount small enough that the answer won't have sufficient resolution. I think DEFAULT_DECIMALS is good enough for that.

@@ -171,6 +171,7 @@ export const registerScaledPriceAuthority = async (
]),
]);

// FIXME can this work without decimalPlaces? if so, read from boardAux
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this usage requires decimalPlaces. (cc @michaelfig)

// FIXME can this work without decimalPlaces? if so, read from boardAux

This seems backwards. If it can work w/o dp, then it should. If it can't, then it needs an alternative source like boardAux.

Copy link
Member

Choose a reason for hiding this comment

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

I think my description of "unit amounts" as decimalPlaces is causing confusion. I wrote that description when the only source of "unit amount" information was getDisplayInfo().decimalPlaces. The code below can be made to work without getDisplayInfo(), if some alternative like boardAux conveys the same information. But the functionality provided by the code needs some source of "unit amounts".

@@ -171,6 +171,7 @@ export const registerScaledPriceAuthority = async (
]),
]);

// FIXME can this work without decimalPlaces? if so, read from boardAux
Copy link
Member

Choose a reason for hiding this comment

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

I think my description of "unit amounts" as decimalPlaces is causing confusion. I wrote that description when the only source of "unit amount" information was getDisplayInfo().decimalPlaces. The code below can be made to work without getDisplayInfo(), if some alternative like boardAux conveys the same information. But the functionality provided by the code needs some source of "unit amounts".

packages/inter-protocol/src/proposals/addAssetToVault.js Outdated Show resolved Hide resolved
@turadg turadg force-pushed the 9220-deprecate-getDisplayInfo branch from f9e0b24 to 028bc5a Compare October 7, 2024 21:37
Copy link

cloudflare-workers-and-pages bot commented Oct 7, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: fff76f3
Status: ✅  Deploy successful!
Preview URL: https://d382aa1b.agoric-sdk.pages.dev
Branch Preview URL: https://9220-deprecate-getdisplayinf.agoric-sdk.pages.dev

View logs

@turadg
Copy link
Member Author

turadg commented Oct 7, 2024

I've punted removal to #10235 and left this one as just about deprecation

@turadg turadg force-pushed the 9220-deprecate-getDisplayInfo branch from 4413fb6 to fff76f3 Compare October 7, 2024 21:47
@turadg turadg marked this pull request as ready for review October 7, 2024 21:47
@turadg turadg requested a review from a team as a code owner October 7, 2024 21:47
@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Oct 7, 2024
@mergify mergify bot merged commit b232bfe into master Oct 8, 2024
101 checks passed
@mergify mergify bot deleted the 9220-deprecate-getDisplayInfo branch October 8, 2024 00:12
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.

deprecate getDisplayInfo
4 participants