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

feat!: inter auction status command #7454

Merged
merged 2 commits into from
Apr 20, 2023
Merged

feat!: inter auction status command #7454

merged 2 commits into from
Apr 20, 2023

Conversation

dckc
Copy link
Member

@dckc dckc commented Apr 19, 2023

closes: #7266

note the revised scope

Description

Add an agops inter auction status command.

Security/ Scaling Considerations

none?

Documentation Considerations

BREAKING CHANGE: this replaces an inter liquidation status that was kinda goofy.

I'm not sure how nextDescendingStepTime works. More on that in a comment below...

Testing Considerations

The only test is a help text snapshot test.

@dckc dckc requested a review from turadg April 19, 2023 01:28
@dckc dckc marked this pull request as draft April 19, 2023 01:28
@dckc
Copy link
Member Author

dckc commented Apr 19, 2023

conflicts already?! I just branched from master a few hours ago. Going to draft mode until I get those worked out...

@dckc
Copy link
Member Author

dckc commented Apr 19, 2023

@Chris-Hibbert The revised scope scope calls for "Time until next price step". nextDescendingStepTime sure seems like it should provide that. I would expect it to change as often as currentPriceLevel, but it doesn't. It seems to always agree with nextStartTime.

For example:

$ date -u && inter auction status
Wed Apr 19 01:30:34 AM UTC 2023
{
  "schedule": {
    "activeStartTime": "2023-04-19T01:30:02.000Z",
    "nextStartTime": "2023-04-19T01:35:02.000Z",
    "nextDescendingStepTime": "2023-04-19T01:30:32.000Z"
  },
  "book0": {
    "startPrice": "9.99 IST/IbcATOM",
    "startProceedsGoal": null,
    "proceedsRaised": null,
    "startCollateral": "0 IbcATOM",
    "collateralAvailable": "0 IbcATOM",
    "currentPriceLevel": "9.4905 IST/IbcATOM"
  },
  "params": {
    "DiscountStep": "5.00%",
    "ClockStep": "00:00:10",
    "LowestRate": "45.00%"
  }
}


$ date -u && inter auction status
Wed Apr 19 01:30:42 AM UTC 2023
{
  "schedule": {
    "activeStartTime": "2023-04-19T01:30:02.000Z",
    "nextStartTime": "2023-04-19T01:35:02.000Z",
    "nextDescendingStepTime": "2023-04-19T01:30:42.000Z"
  },
  "book0": {
    "startPrice": "9.99 IST/IbcATOM",
    "startProceedsGoal": null,
    "proceedsRaised": null,
    "startCollateral": "0 IbcATOM",
    "collateralAvailable": "0 IbcATOM",
    "currentPriceLevel": "8.991 IST/IbcATOM"
  },
  "params": {
    "DiscountStep": "5.00%",
    "ClockStep": "00:00:10",
    "LowestRate": "45.00%"
  }
}

cc @rowgraus @otoole-brendan

@dckc dckc force-pushed the 7266-auction-status-cli branch from 606137f to d8641c4 Compare April 19, 2023 01:34
@dckc
Copy link
Member Author

dckc commented Apr 19, 2023

... I would expect it to change as often as currentPriceLevel, but it doesn't.

oops. yes it does. never mind :)

@dckc dckc force-pushed the 7266-auction-status-cli branch from d8641c4 to 8d81620 Compare April 19, 2023 02:12
@dckc dckc marked this pull request as ready for review April 19, 2023 02:18
@dckc dckc force-pushed the 7266-auction-status-cli branch from 8d81620 to 45b8e47 Compare April 19, 2023 03:27
@dckc
Copy link
Member Author

dckc commented Apr 19, 2023

The only test is a help text snapshot test.

hm... I ended up writing a more extensive test for #7295; maybe I should move it to this PR.

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

more extensive test for #7295; maybe I should move it to this PR.

I support that since the other PR is about docs. But I'd suggest having the best known tests in both and it'll reach master in whichever lands first.

packages/agoric-cli/src/commands/inter.js Outdated Show resolved Hide resolved
packages/agoric-cli/src/commands/inter.js Show resolved Hide resolved

/**
* ref publishSchedule()
* packages/inter-protocol/src/auction/scheduler.js#L103
Copy link
Member

Choose a reason for hiding this comment

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

this line number will be wrong soon enough that it should be omitted. Same for the source path less urgently. For a durable ref try,

Suggested change
* packages/inter-protocol/src/auction/scheduler.js#L103
* @see (import('@agoric/inter-protocol/src/auction/scheduler.js')} publishSchedule

I don't think our tools error on bad paths there yet but they will

Copy link
Member

Choose a reason for hiding this comment

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

oh but more importantly… none of these types should be defined here. The contract must declare its published types and it does.

/**
* @typedef {object} ScheduleNotification
*
* @property {Timestamp | undefined} activeStartTime start time of current
* auction if auction is active
* @property {Timestamp} nextStartTime start time of next auction
* @property {Timestamp} nextDescendingStepTime when the next descending step
* will take place
*/

/**
* @typedef {object} BookDataNotification
*
* @property {Ratio | null} startPrice identifies the priceAuthority and price
* @property {Ratio | null} currentPriceLevel the price at the current auction tier
* @property {Amount<'nat'> | null} startProceedsGoal The proceeds the sellers were targeting to raise
* @property {Amount<'nat'> | null} remainingProceedsGoal The remainder of
* the proceeds the sellers were targeting to raise
* @property {Amount<'nat'> | undefined} proceedsRaised The proceeds raised so far in the auction
* @property {Amount<'nat'>} startCollateral How much collateral was
* available for sale at the start. (If more is deposited later, it'll be
* added in.)
* @property {Amount<'nat'> | null} collateralAvailable The amount of collateral remaining
*/

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I did just a tiny bit of looking for them, but didn't see them when I hovered over the point of publication.

Copy link
Member

Choose a reason for hiding this comment

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

yeah there's some type flow regression in makeRecorderKit. I'll look into it.

Copy link
Member

Choose a reason for hiding this comment

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

fix in 68d4170

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I applied that in this PR. Didn't manage to preserve authorship, though.

@Chris-Hibbert
Copy link
Contributor

This all looks good to me. I won't approve since @turadg is the listed reviewer.

);
return { ...amounts, price };
/** @param {bigint} bp */
const basisPoints = n => `${(Number(n) / 100).toFixed(2)}%`;
Copy link
Member

Choose a reason for hiding this comment

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

sorry I omitted the body from the earlier suggestion.

Suggested change
const basisPoints = n => `${(Number(n) / 100).toFixed(2)}%`;
const basisPoints = bp => `${(Number(bp) / 100).toFixed(2)}%`;

Comment on lines 290 to 307
*
* @typedef {import('@agoric/time/src/types.js').TimestampRecord} TimestampRecord
* @typedef {{
* activeStartTime?: TimestampRecord
* nextStartTime: TimestampRecord,
* nextDescendingStepTime: TimestampRecord,
* }} ScheduleUpdate
*
* @typedef {{
* startPrice: Ratio | null,
* startProceedsGoal: Amount<'nat'> | null,
* proceedsRaised: Amount<'nat'> | null,
* startCollateral: Amount<'nat'>,
* collateralAvailable: Amount<'nat'> | null,
* currentPriceLevel: Ratio | null,
* }} BookUpdate
*/
/** @type { [ScheduleUpdate, BookUpdate, *] } */
Copy link
Member

Choose a reason for hiding this comment

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

I see another push after this discussion but I don't see the types imported.

Suggested change
*
* @typedef {import('@agoric/time/src/types.js').TimestampRecord} TimestampRecord
* @typedef {{
* activeStartTime?: TimestampRecord
* nextStartTime: TimestampRecord,
* nextDescendingStepTime: TimestampRecord,
* }} ScheduleUpdate
*
* @typedef {{
* startPrice: Ratio | null,
* startProceedsGoal: Amount<'nat'> | null,
* proceedsRaised: Amount<'nat'> | null,
* startCollateral: Amount<'nat'>,
* collateralAvailable: Amount<'nat'> | null,
* currentPriceLevel: Ratio | null,
* }} BookUpdate
*/
/** @type { [ScheduleUpdate, BookUpdate, *] } */
*/
/** @type { [ScheduleNotification, BookDataNotification, *] } */

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I'm still working on it. thanks for watching carefully

Copy link
Member Author

Choose a reason for hiding this comment

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

take another look, please? I hope you don't mind that I squashed all the fixups

Copy link
Member

Choose a reason for hiding this comment

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

small enough that it's easy to fully re-review

@dckc dckc force-pushed the 7266-auction-status-cli branch 2 times, most recently from bd4029b to 549ad7d Compare April 19, 2023 23:02
@dckc dckc requested a review from turadg April 19, 2023 23:03
@dckc dckc added automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR labels Apr 19, 2023
@dckc
Copy link
Member Author

dckc commented Apr 20, 2023

darn it... exit 2 from: make scenario2-run-rosetta-ci'

@dckc dckc force-pushed the 7266-auction-status-cli branch from eb2380d to 0125e36 Compare April 20, 2023 16:36
@dckc dckc force-pushed the 7266-auction-status-cli branch from 5cbbfc5 to d7cd994 Compare April 20, 2023 17:19
@mergify mergify bot merged commit 6e56e21 into master Apr 20, 2023
@mergify mergify bot deleted the 7266-auction-status-cli branch April 20, 2023 17:45
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 bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CLI] Auction Status interface
3 participants