-
Notifications
You must be signed in to change notification settings - Fork 215
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: connect auctioneer governance to EC charter #7305
Conversation
I don't seem to be able to pause offers when testing this out locally. Repro steps:
Here are the chain logs, if it's helpful to try and rule out an issue with the voting proposal sent by the UI: https://gist.github.com/samsiegart/4b2dae7d02e9f973d3688c6afab0387b |
@samsiegart in your logs I see
offer with "make bid"
|
@samsiegart meanwhile, the fact that you were able to get a vote passed using the auctioneer contract instance shows that this PR is doing what it's designed to do. |
dbf8e51
to
944b198
Compare
@dckc Perfect, changing to 'new bid' fixes it and I'm seeing |
brand: { | ||
produce: { timer }, | ||
}, |
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.
TimerBrand
has a different API from ERTP Brand
s. Are you sure that no one will try to send getDisplayInfo()
to all the brands in this list? I'm suspicious that this should get a different top-level category.
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.
|
||
t.log('accept charter invitation'); | ||
{ | ||
const instance = agoricNamesRemotes.instance.econCommitteeCharter; |
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.
not good to have two variables with the same name and overlapping scopes. Can this be committee
and the other be auctioneer
?
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 scopes don't overlap. That's the purpose of the {
on 299: to introduce a new scope so that I don't have to think of novel names.
But ok, since you supplied novel names, will do.
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 can no longer see the code (I thought github was supposed to make it straightforward to look at older versions of a PR) so I can no longer prove it, but I thought what I saw was the second scope was fully contained within the first, which is the "not good" form of overlap I was talking about. Did I mis-read it? Were the scopes fully non-overlapping?
- test: propose change to auction governance param
3768d44
to
af1b253
Compare
refs: #7249, #6930, Agoric/dapp-econ-gov#34
Description
To get more frequent auctions for testing (#7249), I tried using governance to change the
StartFrequency
parameter but discovered a couple gaps that we fill here:Security Considerations
closes an important governance gap
Scaling Considerations
none?
Documentation Considerations
agoricNames
is an important API. cf. #7229@michaelfig and talked about
agoricNames.timerBrand.blockTime
sincetimerBrand
is not an ERTP brand. It's been a while since I added a child toagoricNames
, though, so while I look into that, perhaps folks can consider whether this is close enough.Testing Considerations
more fun with bootstrap tests!