node: change aptos watcher to use a graphql indexer #4505
Conversation
There was a problem hiding this comment.
overall seems reasonable, though I'm not sure if tilt will pass. mostly nits. I could go either way about reobserving direcly by version or keeping it by event sequence. I think the obvious advantage of using the version is that reobservation would not require the indexer and that would probably be a good thing.
panoel
force-pushed
the
aptos-watcher-uses-graphql-indexer
branch
from
3439ae9 to
b16e7d0
Compare
panoel
force-pushed
the
aptos-watcher-uses-graphql-indexer
branch
from
cd7e7f7 to
a246b22
Compare
There was a problem hiding this comment.
had limited time to take a look but this seems broadly correct. the main thing I am unsure of and would want confirmed is the security of the new fields we are checking in the transactions/by_version response - type being the most critical one to enforce that we're only picking up legitimate Wormhole messages.
| // Only aptosIndexerRPC is required for indexer mode, token is optional | ||
| useIndexerMode := *aptosIndexerRPC != "" | ||
|
|
||
| // Log which mode will be used |
There was a problem hiding this comment.
nice, this will be critical to debugging. do you think this is worth a (really short) feature flag in the heartbeat as well?
| } | ||
|
|
||
| // processTransactionVersion fetches a transaction by version and extracts WormholeMessage events | ||
| func (e *Watcher) processTransactionVersion( |
There was a problem hiding this comment.
I think this function's logged errors should include a unique field that we can do lookups on in order to debug. Maybe hash listed here? I'm not sure if there's a better field though:
https://aptos.dev/rest-api/operations/get_transaction_by_version
Otherwise, we just get the error strings but we can't tell where they came from.
| txResult := gjson.ParseBytes(txData) | ||
|
|
||
| // Look for WormholeMessage in the events array | ||
| whEventType := fmt.Sprintf("%s::state::WormholeMessage", e.aptosAccount) |
There was a problem hiding this comment.
nit: This could be defined when the watcher is configured instead of inside of this function
|
|
||
| // Check for GraphQL errors in the response | ||
| // The indexer may return errors with a 200 status code | ||
| if gjson.Valid(string(body)) { |
There was a problem hiding this comment.
We always expect valid JSON from this query, right? So I think if this check fails, the function should return nil, err. Right now it will fallthrough to body, nil
| if gjson.Valid(string(body)) { | ||
| response := gjson.ParseBytes(body) | ||
| errors := response.Get("errors") | ||
| if errors.Exists() && errors.IsArray() && len(errors.Array()) > 0 { |
There was a problem hiding this comment.
Similarly the bounds check on errors is worth a loud comment as it will cause a panic if removed
| continue | ||
| } | ||
|
|
||
| if blockHeight.Exists() { |
There was a problem hiding this comment.
nit: It seems like this check should happen before the Uint() check above. It also seems like if the block height doesn't exist for whatever reason then we would never see a health check. Probably if this field does not exist or is 0 after Uint(), an error should be logged.
No description provided.