-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feature/builtin asset faucet #2027
Conversation
fd3514f
to
6cea765
Compare
system-tests failed.
|
system-tests failed.
|
This PR is chunky enough that it needs a @peterbarrow / @EVODelavega review rather than my cursory read. But it sounds nice. |
system-tests failed.
|
system-tests failed.
|
1b7be1e
to
6154eac
Compare
|
||
waitSig(ctx, f.log) | ||
|
||
err = fct.Stop() |
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.
Without looking at the waitSig
thing, I'm assuming this is listening for a kill signal, and so this works, but having fct.Start()
called in a routine, and fct.Stop()
outside of it makes it look like the routine may not have started by the time stop is called. Perhaps that's just because I tend to pass in stop callbacks to the signal handler in some way shape or form.
if len(e.nodes) <= 0 { | ||
e.mu.RUnlock() | ||
return false | ||
} | ||
node := e.nodes[h%uint64(len(e.nodes))] | ||
e.mu.RUnlock() | ||
return node.node == e.self |
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 assuming e.self
is a read-only field, so unlocking the mutex before that shouldn't be a problem. In case a race detector starts yapping about this, using a defer here shouldn't be a problem I don't think (with 1.14 optimising it anyway)
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.
LGTM - if the system tests have been updated, I'm ok to merge this
b48bc6c
to
78517cb
Compare
system-tests failed.
|
1 similar comment
system-tests failed.
|
6997d8a
to
123762a
Compare
system-tests failed.
|
… collateral this enable us to have market with asset ID and use IDs in the collateral
Minor formatting and spelling changes
Provide -> Provides
…e time of the block
… conf with chmod 0600
77fa772
to
da355f5
Compare
@edd done. |
system-tests failed.
|
Implements a faucet for the builtin assets
Remove NotifyTraderAccount
Replace assets strings for Asset types in most graphql types.
Use Asset Ids in all vega instead of the dual ID/Symbol thing.
closes #1522