-
Notifications
You must be signed in to change notification settings - Fork 208
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(auction): auction publish all bids off-chain #7618
Conversation
Draft until the release is in the books. |
f29eab8
to
32360d6
Compare
32360d6
to
ed21579
Compare
5420a64
to
223451a
Compare
@turadg, there's a test failure here in |
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.
initial feedback...
const [scheduleNode, bidsNode] = await Promise.all([ | ||
bookNodeP, | ||
E(bookNodeP).makeChildNode('schedule'), | ||
E(bookNodeP).makeChildNode('bids'), |
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.
something goofy is happening with bidsNode
. see the snapshot...
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.
fixed. rearranged the tree so bids are where they should have been.
for (const r of records.values()) { | ||
this.self.publishOffer(r); | ||
} |
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.
Before we land this, let's make sure we don't do O(n) work like this.
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.
done.
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.
Also, I don't think this handles the case when bids go away.
Until we have an explicit delete method in vstorage (#7405) I think setValue('')
has essentially the same effect.
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.
done in deleteNodeIfPresent
return E(getBidDataRecorder(key)).write( | ||
harden({ | ||
bidScaling: record.bidScaling, | ||
wanted: record.wanted, |
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.
the product spec calls this maxBuy
. Should we
- change it here (my preference)
- translate the field name on the client
- ask product if
wanted
is a good name?
cc @turadg
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.
maxBuy
sounds like the total (original) cap. What's printed here is internally known in some places as stillWant
, reflecting that it's a descending indicator of the amount this bid still needs to be complete.
I will publish both originalWant and stillWant, so the CLI can choose to display either or both.
wanted: record.wanted, | ||
exitAfterBuy: record.exitAfterBuy, | ||
timestamp: record.timestamp, | ||
balance: record.seat.getCurrentAllocation().Bid, |
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.
the product spec calls this give
. Should we
- change it here
- translate the field name on the client
- ask product if
balance
is a good name (my preference)
cc @turadg
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'll leave this as is, since I agree. We can make the change here later if that's called for, or in the CLI.
cbc8e62
to
c47eec7
Compare
Corrected vstorage paths to match:
|
1a4d221
to
0bac932
Compare
Lint test is failing, but not reporting a particular problem. It passes locally. I think this is ready for review. |
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 see any critical problems.
I'd like to look closer... for example to make sure the types work for the CLI. But auction CLI work is sort of on my back burner now, so maybe it's time to land this. Thoughts, @turadg ?
/** | ||
* @typedef {object} BidDataNotification | ||
* | ||
* @property {Array<ScaledBidData>} scaledBids | ||
* @property {Array<PricedBidData>} pricedBids | ||
*/ | ||
export const BidDataNotificationShape = { | ||
scaledBids: M.arrayOf(M.any()), | ||
pricedBids: M.arrayOf(M.any()), | ||
}; | ||
harden(BidDataNotificationShape); |
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 suspect this is dead code.
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.
If you mean BidDataNotificationShape
, it's used on lines 229 and 235.
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'm still missing something. used in what way? Do we actually publish anything at bidsNode
? I don't see any code that publishes data in this shape.
While working on the CLI, I found that this shape actually matched what's published; I'd like to import it from the contract code (ideally in a client helper module a la params.js
that doesn't pull in the whole contract).
agoric-sdk/packages/inter-cli/src/commands/auction.js
Lines 17 to 42 in 6045ee1
const bidData = harden({ | |
timestamp: TimeStampShape, | |
sequence: M.nat(), | |
balance: NatAmountShape, | |
wanted: NatAmountShape, | |
exitAfterBuy: M.boolean(), | |
}); | |
const shapeLeaves = harden({ | |
ScaledBidData: { | |
bidScaling: RatioShape, | |
...bidData, | |
}, | |
PricedBidData: { | |
price: RatioShape, | |
...bidData, | |
}, | |
}); | |
const shape = harden({ | |
...shapeLeaves, | |
BidDataNotification: M.arrayOf( | |
M.or(shapeLeaves.ScaledBidData, shapeLeaves.PricedBidData), | |
), | |
}); |
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 may not understand how to use this right, but getPublicTopics()
returns this node, which is the parent of all the actual bid nodes.
I did move BidDataNotificationShape
to offerBook.js, and used it to declare the type of those sub-nodes that represent the individual bids.
edit to add: This is mostly wrong. revising....
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.
Fixed. Separated BidsDataNotificationShape
from BidDataNotificationShape
.
bidDataKits.delete(key); | ||
} | ||
void deleteNodeIfPresent(key); |
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.
it seems a bit fragile to have to do both of these delete operations here. It would be nice to wrap the bidDataKits
collection in something that automatically updates the storageNodes as appropriate. But maybe the values in the collection aren't enough info to update the storage nodes. hm.
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 would have liked to just call delete(key)
, but that is currently only called when it's known that the record and node were actually created. Aah! I suppose I can put the conditional in that fn at no risk and little cost.
@@ -14,8 +14,8 @@ import { makeOffer } from '@agoric/zoe/test/unitTests/makeOffer.js'; | |||
import { setup } from '@agoric/zoe/test/unitTests/setupBasicMints.js'; |
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'd like to see tests of adding and deleting (exiting) bids that show that vstorage is updated correctly.
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.
Already added. Check out test/auction/snapshots/test-auctionContract.js.md
. The test multiple bidders at one auction step shows that bid1001
has an empty node, while bid1002
and bid1003
show their data.
9a35b14
to
0c50c06
Compare
Includes deleting the content for a bid when it is removed.
0c50c06
to
79a4753
Compare
33d8e17
to
45fe766
Compare
ec9fcec
to
9c2ed56
Compare
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.
putting this in the background for a bit...
Closing to clean up project views |
I'm surprised. This still seems valuable. We've noticed its absence. Don't we want this to be in a list for PM to be able to prioritize? |
The issue is still open: This is a PR. |
yeah, the issue is still available to prioritize. And it has a link to this PR so when it is the PR can be reopened. |
closes: #7906
Description
The auctionBooks will publish all active bids to off-chain storage.
Security Considerations
None.
Scaling Considerations
The means that information that could be accessed on-chain or off will now be more accessible statically.
Documentation Considerations
N/A
Testing Considerations
tested in unit tests.