(VDB-1202) OSM change event transformer#130
Conversation
8f471fb to
4cf549e
Compare
| ) | ||
|
|
||
| // TODO update when real event log exists | ||
| var _ = XDescribe("OsmChange EventTransformer", func() { |
There was a problem hiding this comment.
As you can see, I wasn't able to find any change events in KovanEtherscan. I think it makes sense that none have happened so far, since the change event only gets fired when the OSM contract wants to switch to using a different data-source. And that doesn't seem like something that would happen in the normal course of events, but lmk if I'm thinking about it wrong
There was a problem hiding this comment.
Makes sense to me but I think we probably should double check Mainnet - I think the integration tests are pointed there now, no?
rmulhol
left a comment
There was a problem hiding this comment.
LGTM!
Only blocker I'd say is renaming address_id so that it's more clear that it references the new src
| address_id INTEGER NOT NULL REFERENCES public.addresses (id) ON DELETE CASCADE, | ||
| UNIQUE (header_id, log_id) | ||
| ); | ||
|
|
There was a problem hiding this comment.
COMMENT ON TABLE maker.osm_change
IS E'Note event emitted when change is called on OSM/PIP contract(s)';
would make my life 0.01% easier with the doc changes 😅
There was a problem hiding this comment.
Sorry to switch directions but now I'd ask you to remove that 😅
Instead, we'll handle docs via postgraphile config: https://github.com/makerdao/vdb-postgraphile/pull/4
| id SERIAL PRIMARY KEY, | ||
| log_id BIGINT NOT NULL REFERENCES public.event_logs (id) ON DELETE CASCADE, | ||
| header_id INTEGER NOT NULL REFERENCES public.headers (id) ON DELETE CASCADE, | ||
| address_id INTEGER NOT NULL REFERENCES public.addresses (id) ON DELETE CASCADE, |
There was a problem hiding this comment.
Wondering if we want to have two address IDs on this table? Thinking that users would probably want to be able to filter by which OSM contract had the change in addition to what the VAL address was changed to.
I think we could currently do that via the log_id (filtering on a nested object) but not sure whether we want to depend on that in a case where we know up front that we're expecting multiple contracts to emit this event.
Also maybe worth renaming to src_address_id either way so that it's more clear which one we're referring to.
Was thinking the same thing for LogValue but can take care of that elsewhere.
There was a problem hiding this comment.
🤦♂ Yeah definitely. I think if we follow convention here, we'll use address_id to specify what contract emitted the event, and just call the src column src, even though it's actually a FK-- but definitely open to calling it src_address_id instead. Could also consider making a more widespread change, to clarify the names of address FKs
There was a problem hiding this comment.
Just src is totally fine w/ me 👍
| jug_init.EventTransformerInitializer, | ||
| log_value.EventTransformerInitializer, | ||
| new_cdp.EventTransformerInitializer, | ||
| osm_change.EventTransformerInitializer, |
| github.com/makerdao/vulcanizedb v0.0.12-rc.1.0.20200218211058-5915847f6da9 | ||
| github.com/mattn/go-runewidth v0.0.6 | ||
| github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect | ||
| github.com/modern-go/reflect2 v1.0.1 // indirect |
There was a problem hiding this comment.
These deps just keep coming and going :/
Not opposed to keeping this diff but it'd be cool if we could figure out what causes them to get re-added and make an informed decision about whether they should stay or go
| return nil, err | ||
| } | ||
|
|
||
| src := log.Log.Topics[2].Hex() |
There was a problem hiding this comment.
Is Topics[1] here the msg.sender? If so, maybe worth tossing that on the table as well?
Definitely not a requirement but don't see any downside
| func JugInitSignature() string { return getLogNoteTopicZero(jugInitMethod()) } | ||
| func LogValueSignature() string { return getEventTopicZero(logValueMethod()) } | ||
| func NewCdpSignature() string { return getEventTopicZero(newCdpMethod()) } | ||
| func OsmChangeSignature() string { return getLogNoteTopicZero(osmChangeMethod()) } |
There was a problem hiding this comment.
Could you direct me to where in the code it's referred to as OSM rather than PIP? Still waffling on what name we should use and thinking it's more relevant if we're embedding that name in the migrations etc (as opposed to LogValue where we just left it off entirely)
There was a problem hiding this comment.
https://github.com/makerdao/osm/blob/master/src/osm.sol#L34
Yeah I'm def open to using either one. It's not really clear to me which one would be more useful to the end user, but left to my own devices I'd probably just refer to things as they're referred to in the code. E.g. PIP in the context of the Spot contract, and OSM in the context of the OSM contract.
e2a436f to
0a3debc
Compare
No description provided.