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

fix(action): handle missing Collateral allocation #7195

Merged
merged 2 commits into from
Mar 22, 2023
Merged

Conversation

dckc
Copy link
Member

@dckc dckc commented Mar 21, 2023

refs: #6930

Description

Fix code that assumed getCurrentAllocation() would include an amount in the .Collateral property.
Designed in collaboration with @Chris-Hibbert

Testing Considerations

probably merits a regression test.

@dckc dckc requested review from turadg and Chris-Hibbert March 21, 2023 00:07
@datadog-full-agoric
Copy link

datadog-full-agoric bot commented Mar 21, 2023

Datadog Report

Branch report: dc-auction-fix1
Commit report: 93caaf3

agoric-sdk: 0 Failed, 0 New Flaky, 3594 Passed, 55 Skipped, 21m 49.15s Wall Time

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.

Change LGTM.

probably merits a regression test.

Agree. More general fix could be for "getCurrentAllocation" to be more honest about the possible return values so that this would have been caught statically. Fine my be to make that a ticket for the queue since it will probably require many assertions.

@Chris-Hibbert
Copy link
Contributor

Add

  const seat3 = await driver.bidForCollateralSeat(
    currency.make(210n),
    collateral.make(200n),
  );

after seat2 in 'multiple bidders at one auction step' in test-auctionContract.

@turadg
Copy link
Member

turadg commented Mar 21, 2023

More general fix could be for "getCurrentAllocation" to be more honest about the possible return values so that this would have been caught statically. Fine my be to make that a ticket for the queue since it will probably require many assertions.

I tried this and it does spawn many errors. Filed away as #7198

@dckc dckc force-pushed the dc-auction-fix1 branch 2 times, most recently from 818edfb to 0d4d5a9 Compare March 21, 2023 17:06
@dckc dckc marked this pull request as ready for review March 21, 2023 17:06
@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label Mar 21, 2023
dckc added 2 commits March 22, 2023 13:09
```
2023-03-20T23:18:07.574Z SwingSet: ls: v40: Error: "leftAmount" (an undefined) must be a pass-by-copy record, not "undefined"
 at construct ()
 at Error (/bundled-source/.../node_modules/ses/src/error/tame-error-constructor.js:56)
 at makeError (/bundled-source/.../node_modules/ses/src/error/assert.js:242)
 at fail (/bundled-source/.../node_modules/ses/src/error/assert.js:370)
 at assertRecord (.../marshal/src/typeGuards.js:70)
 at checkLRAndGetHelpers (.../ertp/src/amountMath.js:145)
 at isGTE (.../ertp/src/amountMath.js:190)
 at
 settleAtNewRate (.../inter-protocol/src/auction/auctionBook.js:356)
```

test
the `any` type for `zcf` cascaded to the rest of the function
@dckc dckc force-pushed the dc-auction-fix1 branch from 0d4d5a9 to 479103e Compare March 22, 2023 18:09
@dckc dckc added the bypass:integration Prevent integration tests from running on PR label Mar 22, 2023
@mergify mergify bot merged commit 4874362 into master Mar 22, 2023
@mergify mergify bot deleted the dc-auction-fix1 branch March 22, 2023 18:35
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.

3 participants