-
Notifications
You must be signed in to change notification settings - Fork 220
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
initial inter liquidation bidding CLI #7132
Conversation
Datadog ReportBranch report: ❌ ❌ Failed Tests (11)
|
e706981
to
478dc43
Compare
Datadog ReportBranch report: ❌ ❌ Failed Tests (1)
|
5ad081f
to
302c5d5
Compare
Run auctions every 2 min for testingThese are the edits I make to tweak the parameters:
Tracing Liquidation and Auctionsedits I use``` diff --git a/packages/inter-protocol/src/auction/auctionBook.js b/packages/inter-protocol/src/auction/auctionBook.js index dec1e039b..80c28734e 100644 --- a/packages/inter-protocol/src/auction/auctionBook.js +++ b/packages/inter-protocol/src/auction/auctionBook.js @@ -48,7 +48,7 @@ const DEFAULT_DECIMALS = 9; * added in the appropriate place and settled when the price reaches that level. */-const trace = makeTracer('AucBook', false); /**
const { Fail, quote: q } = assert; -const trace = makeTracer('Auction', false); /**
diff --git a/packages/inter-protocol/src/vaultFactory/liquidation.js b/packages/inter-protocol/src/vaultFactory/liquidation.js import { AUCTION_START_DELAY, PRICE_LOCK_PERIOD } from '../auction/params.js'; -const trace = makeTracer('LIQ', false); /** @typedef {import('@agoric/time/src/types').TimerService} TimerService /
diff --git a/packages/inter-protocol/src/vaultFactory/vaultManager.js b/packages/inter-protocol/src/vaultFactory/vaultManager.js const { details: X, Fail } = assert; -const trace = makeTracer('VM', false); /** @typedef {import('./storeUtils.js').NormalizedDebt} NormalizedDebt /
|
c6b4aa3
to
f8d0b76
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #7132 +/- ##
==========================================
- Coverage 79.35% 70.91% -8.45%
==========================================
Files 396 450 +54
Lines 74796 86004 +11208
Branches 3 3
==========================================
+ Hits 59355 60990 +1635
- Misses 15440 24948 +9508
- Partials 1 66 +65
|
Datadog ReportBranch report: ✅ |
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 only looked closely at the portion that dealt specifically with adding bids and deposits. It looks fine to me, though I added some comments and questions.
The framework is opaque to me, and I didn't look closely at the rest.
callPipe: [['makeAddCollateralInvitation', []]], | ||
}, | ||
proposal: { give }, | ||
}; |
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 will be optional offerArgs
that look like { toRaise: istAmount }
, coming in #6952. Do you want to add it now?
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.
Good to know it's coming, but I don't quite understand it well enough to add it without being able to test it yet.
const discount = r => | ||
100 - (Number(r.numerator.value) / Number(r.denominator.value)) * 100; | ||
return { amount, record, price, discount }; |
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.
discounts can be more or less than 100. Will this correctly distinguish 105% of oracle price from 95% and 5%?
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.
That's the idea, yes...
There was a unit test in the clientSupport
layer, where the discount is a number in the -1 to 1 range, that tested that -0.10 gives an offerBidScaling
of makeRatio(11n, IST, 10n)
. But adding tests for the 3 cases you ask about showed that things could be tightened up.
Now I have tests for all 4 cases:
const discounts = [
{ cliArg: 0.05, offerBidScaling: makeRatio(95n, ist.brand, 100n) },
{ cliArg: 0.95, offerBidScaling: makeRatio(5n, ist.brand, 100n) },
{ cliArg: -0.05, offerBidScaling: makeRatio(105n, ist.brand, 100n) },
{ cliArg: -0.1, offerBidScaling: makeRatio(110n, ist.brand, 100n) },
];
And I checked at the command line:
---discount=5
gives offerBidScaling
of makeRatio(95n, IST, 100n)
.
---discount=-5
gives offerBidScaling
of makeRatio(105n, IST, 100n)
.
---discount=95
gives offerBidScaling
of makeRatio(5n, IST, 100n)
.
and --discount=950
gives an error:
error: option '--discount [percent]' argument '950' is invalid. must be between -100 and 100
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 transformation with discountRate
in the contract is better ergonomically but I expect confusion in reading the two.
consider calling this +/- expression relativeDiscount
) | ||
.requiredOption( | ||
'--discount [number]', | ||
'bid discount (%)', |
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.
'bid discount (%)', | |
'bid discount or markup (%)', |
.description('Print an offer to bid collateral by price.') | ||
.requiredOption('--price [number]', 'bid price', Number) | ||
.requiredOption('--giveCurrency [number]', 'Currency to give', Number) | ||
.requiredOption('--wantCollateral [number]', 'bid price', Number) |
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.
offer safety is available and sensible on a bid by price. Is it worth adding a separate option to let users separately specify their offer-safety want
? The want in the offerArgs specifies the price, but doesn't provide offer safety.
(Offer safety is also available on bids by discount, though the meaning is more subtle. I haven't thought through the implications, but I know Zoe would enforce it if it was specified.)
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.
Oh. I didn't realize. Let's do that in a follow-on PR, along with cancel / exit.
@@ -0,0 +1,111 @@ | |||
# Snapshot report for `test/test-inter-cli.js` | |||
|
|||
The actual snapshot is saved in `test-inter-cli.js.snap`. |
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.
should the actual snapshot be checked in or .gitignored?
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 read of the ava snapshot testing docs is that you must check in the .snap
file, as that's what determines whether the test passes or fails, and you should check in the .md
file so that you can get a text representation of what changed.
|
||
> Command usage: | ||
|
||
`Usage: inter [options] [command]␊ |
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.
Is there a reason to prefer the "␊" marks in the .md and its rendering? How much work to remove them?
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 tried .replace(/\r/g, '')
but I guess that would be CRs, not LFs. hm.
@turadg I moved the |
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.
Good stuff! None of my suggestions are blocking and I see no automerge tag so I'll approve and leave to your discretion.
@@ -0,0 +1,376 @@ | |||
// @ts-check | |||
import { CommanderError, InvalidArgumentError } from 'commander'; | |||
import { M, matches } from '@agoric/store'; |
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.
regrettable to have cli depend on store. consider a comment here,
// TODO remove dep after https://github.com/Agoric/agoric-sdk/issues/7090
@@ -1,20 +1,23 @@ | |||
#!/usr/bin/env node | |||
/* eslint-disable @jessie.js/no-nested-await */ | |||
/* global process */ | |||
/* global fetch */ |
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 see you preferred import process
to the global. similarly you could have,
import fetch from 'node-fetch';
though I trust the global slightly more than the package import.
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.
node-fetch seems to be a separate package.
The fetch global doesn't seem to be available from a node built-in package the way process
is.
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.
node-fetch seems to be a separate package.
Yes, it implements the window.fetch
API. But it's not as separate as it may seem because this shim is loaded in the bin scripts in this package. IOW node-fetch
is what you're really getting here anyway. Better for it to be explicit, imo, to avoid the confusion demonstrated in this thread.
packages/agoric-cli/src/bin-agops.js
Outdated
stderr: process.stderr, | ||
createCommand, | ||
execFileSync, | ||
clock: () => Date.now(), |
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.
is this common to name the time getter clock
? now()
is more familiar to me fwiw
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.
agreed. will change to now
packages/agoric-cli/src/bin-agops.js
Outdated
await program.parseAsync(process.argv).catch(err => { | ||
if (err instanceof CommanderError) { | ||
console.error(err.message); | ||
} else { | ||
console.error(err); // CRASH! show stack trace | ||
} | ||
process.exit(1); | ||
}); |
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.
since we do have top-level await, consider using regular try/catch
await program.parseAsync(process.argv).catch(err => { | |
if (err instanceof CommanderError) { | |
console.error(err.message); | |
} else { | |
console.error(err); // CRASH! show stack trace | |
} | |
process.exit(1); | |
}); | |
try { | |
await program.parseAsync(process.argv); | |
} catch (err) { | |
if (err instanceof CommanderError) { | |
console.error(err.message); | |
} else { | |
console.error(err); // CRASH! show stack trace | |
} | |
process.exit(1); | |
}; |
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.
top-level await
feels weird to me... but it clearly does exactly the same thing. What I want is for main programs to just export main
and have the runtime call it, passing in all I/O access explicitly.
I guess that's why we're building endo. Meanwhile, when in Rome...
{ env, stdout, stderr, clock, execFileSync, createCommand }, | ||
{ fetch }, |
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.
don't you usually put io
powers in one param?
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.
Yes, I usually do. In this case, it seemed like network access and process stuff fit in distinct piles. Or something.
const { body, slots } = JSON.parse(txt); | ||
const record = m.unserialize({ body, slots }); | ||
coalescer.update(record); |
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.
consider a feature like,
const { body, slots } = JSON.parse(txt); | |
const record = m.unserialize({ body, slots }); | |
coalescer.update(record); | |
coalescer.updateSerialized(txt); |
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.
making the coalescer aware of marshaling looks kinda awkward
value, | ||
} = amt; | ||
const { brand, value } = amt; | ||
// @ts-expect-error XXX BoardRemote |
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.
why not type AssetDescriptor?
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 don't understand. AssetDescriptor
is used on line 36.
packages/agoric-cli/src/lib/rpc.js
Outdated
export const networkConfig = | ||
'AGORIC_NET' in process.env && process.env.AGORIC_NET !== 'local' | ||
? await fromAgoricNet(NonNullish(process.env.AGORIC_NET)) | ||
export const getNetworkConfig = async env => |
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.
export const getNetworkConfig = async env => | |
/** @returns {MinimalNetworkConfig} */ | |
export const getNetworkConfig = async env => |
const url = (path = 'published', { kind = 'children', height = 0 } = {}) => | ||
`/abci_query?path=%22/custom/vstorage/${kind}/${path}%22&height=${height}`; | ||
|
||
const readStorage = (path = 'published', { kind = 'children', height = 0 }) => |
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 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.
👏
- agoric-cli: support explicit net, env, stdout authority - update makeAmountFormatter() use of BoardRemote; refine types - nicer diagnostics from vstorage - prune debug logging from readFully - thread invitationBrand thru coalesceWalletState - show stack trace for bugs only - some unit test coverage with mock network etc. - docs snapshot test
refs: #6930
Description
Add an
inter
command with sub-commands forinter bid by-price
etc.Cancel (exit) functionality is to come in a follow-on PR.
Security Considerations
Uses "want" in the same way that the contract does; that is: in a way that suggests, but does not actually provide, offer safety.
Scaling Considerations
The expected userbase is small-ish. Consequences of using it at a larger scale have not been explored.
Documentation Considerations
Brief descriptions of commands and options are provided. test/snapshots/test-inter-cli.js.md
should provide a handy reference as a basis for more thorough end-user docs (#6889).
Some effort went into reasonable diagnostics, distinguished from crashes / stack traces.
Testing Considerations
Unit tests of several different sorts are provided, though coverage is incomplete.
I did some manual testing with a local chain; @raphdev was able to reproduce some of my results.