feat(grpc): transaction retrieval #17
Conversation
| // TODO: ValidateTx only does stateful validation, not execution. This here lets us get the Events in the TxResult. | ||
| res, _, err = g.am.Simulate(ctx, tx) | ||
| if err != nil { | ||
| // Simulate should only return an error at this level, | ||
| // if it failed to get state from the store. | ||
| g.log.Warn("Error attempting to simulate transaction", "route", "simulate_tx", "err", err) | ||
| return nil, fmt.Errorf("failed to simulate transaction: %w", err) | ||
| } |
There was a problem hiding this comment.
This feels wrong to get events. Must be another way or we are missing some call in the am, I need to dive into see how the SDK handles with comet
| txIdx map[string]*TxResultResponse | ||
| txIdxLock sync.Mutex |
There was a problem hiding this comment.
Q: I know sync.Map is typically bad & to stay away, but this may be a good case for it as we only write 1 time and read many (growing cache only) due to unique Txs only?
cc @mark-rushakoff am I right here or should I stick to standard map locks?
https://pkg.go.dev/sync#Map
The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.
There was a problem hiding this comment.
We can't really have an ever-growing cache, right? It's fine as a stub just to get through integrations, but at a minimum we need a // TODO next to it.
Maps and concurrency
The convention I'm most used to seeing would be
txIdxMu sync.Mutex
txIdx map[string]*TxResultResponsemuname for mutexes, scoping the name astxIdxMuis appropriate given the narrow scope of the mutex's responsibility- mutex declared immediately before the fields it protects
I think a plain mutex and a map is a fine combination here. If we expected many more transaction queries than writes, maybe a RWMutex would be a better fit.
My rule of thumb is to avoid sync.Map until you really have evidence it's needed over the typical mutex and map pattern, and I don't think we have that yet. The type casting is the particular annoyance here.
I don't think the map value needs to be a pointer here.
Long-lived, growing cache
While the driver does need to provide the functionality to find a transaction by hash and return it to the user, I would have expected something in the SDK to provide the retrieval functionality directly. Something like AppManager.State.FindTxByHash(h).
If that really doesn't exist (yet) or there is some good reason we can't use it, then let's push the functionality behind some trivial interface, so we can at least trivially switch to an LRU or put it on disk so we don't have to introduce an OOM risk.
| response := getGordianResponseFromSDKResult(res) | ||
|
|
||
| txHash := tx.Hash() | ||
| response.TxHash = strings.ToUpper(hex.EncodeToString(txHash[:])) |
There was a problem hiding this comment.
I think response.TxHash = fmt.Sprintf("%X", txHash) is more appropriate here, so we don't have to do two copies of the string (once encoded and once more to uppercase).
Same below on L111.
| @@ -5,6 +5,7 @@ import ( | |||
| "fmt" | |||
| "log/slog" | |||
| "net" | |||
| sync "sync" | |||
There was a problem hiding this comment.
Just "sync" here of course -- goimports should resolve this automatically, and I think people have tended towards goimports instead of go fmt.
| txIdx map[string]*TxResultResponse | ||
| txIdxLock sync.Mutex |
There was a problem hiding this comment.
We can't really have an ever-growing cache, right? It's fine as a stub just to get through integrations, but at a minimum we need a // TODO next to it.
Maps and concurrency
The convention I'm most used to seeing would be
txIdxMu sync.Mutex
txIdx map[string]*TxResultResponsemuname for mutexes, scoping the name astxIdxMuis appropriate given the narrow scope of the mutex's responsibility- mutex declared immediately before the fields it protects
I think a plain mutex and a map is a fine combination here. If we expected many more transaction queries than writes, maybe a RWMutex would be a better fit.
My rule of thumb is to avoid sync.Map until you really have evidence it's needed over the typical mutex and map pattern, and I don't think we have that yet. The type casting is the particular annoyance here.
I don't think the map value needs to be a pointer here.
Long-lived, growing cache
While the driver does need to provide the functionality to find a transaction by hash and return it to the user, I would have expected something in the SDK to provide the retrieval functionality directly. Something like AppManager.State.FindTxByHash(h).
If that really doesn't exist (yet) or there is some good reason we can't use it, then let's push the functionality behind some trivial interface, so we can at least trivially switch to an LRU or put it on disk so we don't have to introduce an OOM risk.
Summary
For Interchaintest & IBC, we need to be able to get a transaction (by its hash) and verify its data.
Update
I am building on reece/ibc-slop-backup as my work in progress hack together POC for this now. Will be a large diff that will then be broken down into many PRs once something is working.