feat: more usable bidding CLI: integrated sign/broadcast #7256

dckc · 2023-03-28T19:18:34Z

closes: #6930

Description

Rather than just printing an offer, sign and broadcast it, wait for it to appear in a block, and wait for the effect in the wallet state. Give non-zero exit code on errors.

add agops inter bid cancel <id> command (with snapshot reference docs).
for agops inter bid, the "want" defaults to a very high number (1_000_000n). Note: this is not "want" in the Zoe sense; it's more like "maxPurchase"
error handling: when agd is not in $PATH, give a reasonable diagnostic rather than a stack trace
testing convenience: get --keyring-background default from env.AGORIC_KEYRING_BACKGROUND

Security Considerations

not much... though making it less error prone contributes to security

Scaling Considerations

polls every 3 seconds (Nyquist freq of 6 sec block time) when waiting for tx to appear in a block or for wallet state change.

Documentation Considerations

See #7295 for an attempt at somewhat reasonable docs.

agops inter bid cancel has reference docs by way of a snapshot test, as usual.

I didn't add any docs for $AGORIC_KEYRING_BACKGROUND.

Testing Considerations

Significant manual testing, though not all on the same version.

I tested agops inter bid cancel end-to-end on a testnet. Before, agops inter bid list showed:

{"id":"bid-1680023811410","price":"9 IST/IbcATOM","give":{"Currency":"3IST"},"want":"0.2IbcATOM"}

then:

$ agops inter bid cancel bid-1680023811410 >,cancel.json
$ agoric wallet send --from gov1 --offer ,cancel.json

and after, we got our money back:

{"id":"bid-1680023811410","price":"9 IST/IbcATOM","give":{"Currency":"3IST"},"want":"0.2IbcATOM","payouts":{"Currency":"3IST"}}

codecov · 2023-03-28T19:23:12Z

Codecov Report

Merging #7256 (9e6ba5a) into master (3825602) will increase coverage by 0.01%.
The diff coverage is 76.36%.

❗ Current head 9e6ba5a differs from pull request most recent head 88bb40a. Consider uploading reports for the commit 88bb40a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7256      +/-   ##
==========================================
+ Coverage   66.17%   66.19%   +0.01%     
==========================================
  Files         312      313       +1     
  Lines       60018    60127     +109     
  Branches        3        3              
==========================================
+ Hits        39718    39799      +81     
- Misses      20232    20262      +30     
+ Partials       68       66       -2

Impacted Files	Coverage Δ
packages/agoric-cli/src/commands/inter.js	`83.49% <67.50%> (-1.06%)`	⬇️
packages/agoric-cli/src/lib/wallet.js	`69.04% <100.00%> (-1.97%)`	⬇️

... and 3 files with indirect coverage changes

turadg

Good stuff! Approving with assumption that the AGORIC_KEYRING_BACKEND will get into the CLI help as suggested.

packages/agoric-cli/test/test-inter-cli.js

packages/agoric-cli/src/commands/inter.js

turadg · 2023-03-28T19:58:13Z

packages/agoric-cli/src/commands/inter.js

@@ -184,6 +186,9 @@ For example:
    .command('bid')
    .description('auction bidding commands');

+  const sendHint =


if you don't opt for outputActionAndHint at least put this const in lib/wallet. it's about wallet actions.

turadg · 2023-03-28T19:59:13Z

packages/agoric-cli/test/test-inter-cli.js

-        Currency: mk(bslot.IST, 50000000n),
-      },
+      give: { Currency: mk(bslot.IST, 50_000_000n) },
+      want: { Collateral: mk(bslot.ATOM, 5_000_000n) },


what happens if this doesn't match the want in offerArgs?

Good question... @Chris-Hibbert ?

I'd love to see contract unit tests in this area.

I considered adding one but figured maybe I have already spent too much time on this bidding CLI and I should check with some reviewers about how much testing and of what sort is cost-effective here.

It's completely legal for the want in the proposal to be different. The effect is that it specifies a minimum fill for the order. If we think people will want that functionality, we could add support for the auction contract to pay attention. Actually the auction wouldn't do anything different. If it couldn't completely satisfy the want, it would exit the seat.

Currently it just allows Zoe to kill offers when the auction tries to reallocate less than the requested amount. Most of the time, if the want is reasonable, the auction will be likely to surpass it.

packages/inter-protocol/src/clientSupport.js

Chris-Hibbert · 2023-03-28T23:08:55Z

packages/inter-protocol/src/clientSupport.js

    collateralBrand,
    scaleDecimals(opts.wantCollateral),
  );

+  const proposal = {
+    give,
+    ...('price' in opts ? { want: { Collateral: wantAmt } } : {}),


I think want shouldn't be added to the proposal by default. It turns the bid into an all-or-nothing offer. If the auction tries to partially satisfy the offer, zoe will abort the transaction as not-offer-safe.

Help me understand how the contract works in this respect?

Do you have a suggestion on what the CLI should look like?

Help me understand how the contract works in this respect?

The auction has a pool of assets it's trying to sell. There are (hopefully) many buyers who have requested different amounts at varying price levels. The auctioneer serves requested buyers one at a time when their specified price is at or above the current price level. Each offer specifies a maximum amount of money to spend, and a maximum price at which to buy. When the auctioneer looks at one particular offer to buy, it may be able to satisfy it in full (use up all the available spend), or it may only have enough remaining assets to partially satisfy the offer.

The want in the proposal is orthogonal to the amount to spend specified by the offer, and is enforced by Zoe. The auction attempts to transfer to the offer as much of the available assets as it can afford. Since we want to support large outstanding orders that buy assets over time, the normal case is that partially filled orders remain in the book for the next auction. I have a PR in prep that allows offers to be exited immediately on partial fulfillment.

If an offer specifies a want to Zoe, then Zoe will void the transaction if it doesn't give the offer everything it asked for. The contract could proactively look at the want, but the only thing it would be able to do at that point would be to exit the offer early. It has no facility for hanging onto offers that are above the current auction price in order to use them in a future auction. Instead it wants to go on to the next offer and accept it instead.

IRL, an analogous order on stock markets is fill or kill, which must be filled in its entirety or removed from the book.

I hope I'm not over-explaining. Did I at least clarify what you were looking for?

Do you have a suggestion on what the CLI should look like?

I imagine a flag like --addWant <amount>, which would have doc comments explaining that it disallows partial execution. But I'm not very familiar with the style of parameters currently in use. I'd be happy to discuss patterns of use that make sense to you.

The re-use of want from offer safety for the offerArg is a source of confusion. We also need them to be named differently in order for the CLI to take both arguments.

Zoe's want can't be changed but the offerArg can. How about something like maxSpend?

Is there something in the contract that suggested the use of want for this in the CLI? I thought the contract's terminology around this was bidScaling and price.

Is there something in the contract that suggested the use of want for this in the CLI? I thought the contract's terminology around this was bidScaling and price.

agoric-sdk/packages/inter-protocol/src/auction/auctioneer.js

Line 275 in b834e89

want: M.or({ Collateral: AmountShape }, {}),

oops... rather:

agoric-sdk/packages/inter-protocol/src/auction/auctionBook.js

Line 71 in b834e89

{ want: collateralAmountShape },

Sorry, Dan. You're right. The contract specifies that. (oops)

The replacement for want in the offerArgs would be something more like targetPurchase or desiredBuy. I agree that want is bad here. What should we change it to?

desiredBuy is closer to the style of give/want while being distinct enough. also "desired" makes clear that it's not a guarantee the way want is.

dckc · 2023-03-29T00:39:59Z

I suppose this is a flake:

not ok 11 - bootstrapTests › vaults-integration › exit bid %ava-dur=19976ms

https://github.com/Agoric/agoric-sdk/actions/runs/4547397258/jobs/8017314630?pr=7256#step:11:1270

turadg · 2023-03-29T16:18:59Z

I suppose this is a flake:

not ok 11 - bootstrapTests › vaults-integration › exit bid %ava-dur=19976ms

Darn. Have you seen it flake in master or just this PR? Maybe it's due to the offer safety constraint added in makeBidOffer.

turadg · 2023-03-30T22:13:38Z

packages/agoric-cli/src/commands/inter.js

@@ -266,33 +388,68 @@ For example:

  bidCmd
    .command('cancel')
-    .description('Print a request to exit a bid offer')
+    .description('Exit a bid offer')


we have no "exit" operation, only "try exit", which may fail

I wonder how I check whether it failed.

Oh, that's a good one. tryExit() is async and returns nothing. You can call E(seat).hasExited(), and it will promptly return a boolean as to whether it has finished exiting. You can use E(seat).getExitSubscriber() to be notified when it exits. But there's nothing that tells you whether tryExit() succeeded.

And for it to be useful from a CLI (or any other off-chain client), it has to be visible via chainStorage/RPC.

tryExit() is async and returns nothing.

I'm wrong again.

It does return a promise for the result of the attempt to exit.

dckc · 2023-03-31T01:16:58Z

Based on alpha testing yesterday and today, I started expanding the scope of this branch substantially. I wonder if I should expand the scope of this PR or close it. I think I incorporated all the comments.

I suppose I should land just this scope, but that would be extra work. hm.

dckc · 2023-03-31T17:39:47Z

packages/agoric-cli/test/snapshots/test-inter-cli.js.md

+    ␊
+    Options:␊
+      --from <address>  wallet address literal or name␊
+      --generate-only   print wallet action only␊
      -h, --help        display help for command`

 ## Usage: inter reserve add


I noticed we already have a commands/reserve.js. There's no particular reason to treat this as part of a bidding CLI. I better move it.

a separate PR would be better, in fact.

turadg

Has some rough edges and tech debt that don't belong in product code. However, this tool is for testing and it that bar. These features will help a lot so LGTM to merge as is.

turadg · 2023-04-04T22:17:23Z

packages/inter-protocol/src/clientSupport.js

-  const makeCollateral = x =>
-    AmountMath.make(collateralBrand, scaleDecimals(x));
-
+const makeBidOffer = (parseAmount, opts) => {


this makes this offer maker different than all the rest in here. I think we should maintain an ClientOfferMaker function signature. Can't enforce that until we upgrade to TS 5.0 though: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#satisfies-support-in-jsdoc

I won't block on this but please leave an XXX comment or make a new issue to get consistency.

on second thought, in order to address some testing feedback from CH above, I moved parseAmount in to opts so that this one follows the convention.

desiredBuy and wantMinimum are done

dckc · 2023-04-05T15:53:52Z

flake?

not ok 124 - upgrade › upgrade-replay › replay after upgrade %ava-dur=7903ms
  ---
    name: AssertionError
    message: Rejected promise returned by test
    values:
      'Rejected promise returned by test. Reason:': |-
        Error {
          message: 'historical inaccuracy in replay of v2',
        }

https://github.com/Agoric/agoric-sdk/actions/runs/4619796875/jobs/8169140440?pr=7256#step:5:260

because --keyring-backend=test gets boring

BREAKING CHANGE: --giveCurrency option becomes --give and likewise --wantCollateral -> --want. Use snake-case for options throughout to match cosmos-sdk style - `bid` commands integrate `wallet send` step - show bid result - use only liveOffers for inter bid list by default - to show all: --all - fix: provide --allow-spend for tryExitOffer unless/until we change the wallet contract - don't show redundant result in error case - trial: use offer safe want in by-price bids - leave exit onDemand implicit - refactor: - factor out outputActionAndHint - allow explicit io for execSwingsetTransaction - combine options into one object - factor out storedWalletState - factor pollBlocks out of pollTx - update tests feat: inter bid by-discount sends; --generate-only; list --all - bid by-discount, like by-price, sends the tx - factor out placeBid, SharedBidOpts, withSharedBidOptions - bid list uses activeOffers unless --all is given - support --generate-only - use snake-case for option names; avoid [xx] optional syntax - give PATH clue everywhere execFileSync is used - test.todo()s - code polish: - expand file, function docs - refactor: hoist bidInvitationShape

- chore: bid "want" is required but different from proposal.want

- docs: normalizeAddress: keyring type tweak

dckc requested review from turadg, samsiegart, Chris-Hibbert and otoole-brendan March 28, 2023 19:18

turadg approved these changes Mar 28, 2023

View reviewed changes

dckc added automerge:squash Automatically squash merge bypass:integration Prevent integration tests from running on PR labels Mar 28, 2023

Chris-Hibbert previously requested changes Mar 28, 2023

View reviewed changes

dckc removed the automerge:squash Automatically squash merge label Mar 29, 2023

dckc force-pushed the dc-bid-cli branch from 8779ba4 to 88bb40a Compare March 29, 2023 00:40

dckc marked this pull request as draft March 30, 2023 21:59

turadg reviewed Mar 30, 2023

View reviewed changes

dckc changed the title ~~feat: agops inter bid cancel, offer-safe bid by-price~~ WIP: agops inter bid cancel etc. Mar 31, 2023

dckc force-pushed the dc-bid-cli branch 3 times, most recently from 6882ea3 to 4c6a90e Compare March 31, 2023 15:23

mergify bot mentioned this pull request Mar 31, 2023

docs: Inter Protocol Liquidation Bidding Test Tool #7295

Merged

dckc commented Mar 31, 2023

View reviewed changes

dckc force-pushed the dc-bid-cli branch 2 times, most recently from c29b823 to 4983d7e Compare March 31, 2023 19:05

dckc mentioned this pull request Mar 31, 2023

fix: agops: no I/O (errors) until command action #7299

Merged

dckc force-pushed the dc-bid-cli branch 2 times, most recently from a05f324 to fa305ac Compare April 1, 2023 03:51

dckc changed the title ~~WIP: agops inter bid cancel etc.~~ feat: more usable bidding CLI: integrated sign/broadcast Apr 1, 2023

dckc force-pushed the dc-bid-cli branch from fa305ac to 4bdffe2 Compare April 3, 2023 23:12

dckc removed the bypass:integration Prevent integration tests from running on PR label Apr 3, 2023

turadg approved these changes Apr 4, 2023

View reviewed changes

dckc added the automerge:rebase Automatically rebase updates, then merge label Apr 4, 2023

dckc added automerge:rebase Automatically rebase updates, then merge and removed automerge:rebase Automatically rebase updates, then merge labels Apr 4, 2023

dckc force-pushed the dc-bid-cli branch 2 times, most recently from 816d452 to c0fb26b Compare April 5, 2023 14:48

dckc added 18 commits April 5, 2023 16:07

fix(agoric-cli): handle agd not in $PATH

3ad7887

feat(agoric-cli): inter bid cancel

684a2a3

feat: AGORIC_KEYRING_BACKGROUND convenience

89ae6c5

because --keyring-backend=test gets boring

feat: inter vbank list: expose denom, boardId, decimalPlaces

f4fb840

chore: agops inter reserve add -> agops reserve add

b833ba1

- chore: bid "want" is required but different from proposal.want

feat: agops auction proposeParamChange

f9e1f80

feat: agops ec: integrated sign, broadcast, await

e13cf6c

- docs: normalizeAddress: keyring type tweak

refactor: isolate outdated terminology

6b27b87

docs(agoric-cli): sketch POLA re-org for threading I/O powers

6e8a18a

chore: postpone async clock(); just use now()

a4d0256

refactor: move storedWalletState, pollOffer to lib/wallet.js

a3cde50

chore: Bid offer maker takes desiredBuy, wantMinimum

ab40c8c

chore: Offers.auction.Bid takes string amounts and a parser

ef7dca1

chore: inter bid: amount args: --give 5 -> --give 5IST

d743e58

docs: note 1st arg of Offers.auction.Bid is odd

87d951d

chore: Bid WIP

9b2c384

build(inter-protocol): sync verion of agoric pkg

1200ad5

dckc force-pushed the dc-bid-cli branch from c0fb26b to 1200ad5 Compare April 5, 2023 16:07

mergify bot merged commit 013fff8 into master Apr 5, 2023

mergify bot deleted the dc-bid-cli branch April 5, 2023 16:47

dckc mentioned this pull request Apr 5, 2023

[CLI] Bidding interface wantCollateral required when bidding by-price #7268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: more usable bidding CLI: integrated sign/broadcast #7256

feat: more usable bidding CLI: integrated sign/broadcast #7256

dckc commented Mar 28, 2023 •

edited

Loading

codecov bot commented Mar 28, 2023 •

edited

Loading

turadg left a comment

turadg Mar 28, 2023

turadg Mar 28, 2023

dckc Mar 28, 2023

Chris-Hibbert Mar 28, 2023

Chris-Hibbert Mar 28, 2023

dckc Mar 29, 2023

Chris-Hibbert Mar 29, 2023

turadg Mar 29, 2023 •

edited

Loading

Chris-Hibbert Mar 29, 2023

dckc Mar 29, 2023

dckc Mar 29, 2023

Chris-Hibbert Mar 29, 2023

turadg Mar 30, 2023

dckc commented Mar 29, 2023

turadg commented Mar 29, 2023

turadg Mar 30, 2023

dckc Mar 31, 2023

Chris-Hibbert Mar 31, 2023

dckc Mar 31, 2023

Chris-Hibbert Mar 31, 2023

dckc commented Mar 31, 2023

dckc Mar 31, 2023

dckc Mar 31, 2023

turadg left a comment

turadg Apr 4, 2023

dckc Apr 4, 2023

dckc Apr 5, 2023

dckc commented Apr 5, 2023

feat: more usable bidding CLI: integrated sign/broadcast #7256

feat: more usable bidding CLI: integrated sign/broadcast #7256

Conversation

dckc commented Mar 28, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

codecov bot commented Mar 28, 2023 • edited Loading

Codecov Report

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Mar 29, 2023

turadg commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Mar 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Apr 5, 2023

dckc commented Mar 28, 2023 •

edited

Loading

codecov bot commented Mar 28, 2023 •

edited

Loading

turadg Mar 29, 2023 •

edited

Loading