Support chains with no gRPC & with BLS keys #43
Conversation
It would be nice to have explanation in the code (some comment) why this is needed and why parsing raw json (which may or may not be compatible with the hand made struct - reponses can change through versions). Also I don't think it is the best option to switch every network to use json parsing rather than upstream client, just because "bera" is special. I would in fact, leave everything as is and add json parsing for the networks that are "special" (like bera). |
internal/pkg/daemon/checks.go
Outdated
| @@ -36,9 +36,13 @@ func (d *Daemon) preUpgradeChecks( | |||
|
|
|||
| // notify once about the upcoming upgrade | |||
| if currStep == urproto.UpgradeStep_MONITORING { | |||
| chainName := "unsupported-name" | |||
There was a problem hiding this comment.
what is the purpose of that one?
There was a problem hiding this comment.
We can't access nodeInfo if there's no gRPC, this dependency is not necessary, as we already ask for a unique network identifier in the config (UpgradeRegistry.Network)
added an explanation
how would this work though? the current deserialization code is decoding via proto definitions, which also will break if the protocol changes and we don't update it |
| type StatusResponse struct { | ||
| Result struct { | ||
| ValidatorInfo struct { | ||
| Address string `json:"address"` | ||
| VotingPower uint64 `json:"voting_power,string"` | ||
| } `json:"validator_info"` | ||
| NodeInfo struct { | ||
| Network string `json:"network"` | ||
| } `json:"node_info"` | ||
| SyncInfo struct { | ||
| LatestBlockTime string `json:"latest_block_time"` | ||
| LatestBlockHeight int64 `json:"latest_block_height,string"` | ||
| } `json:"sync_info"` | ||
| } `json:"result"` | ||
| ValidatorInfo struct { | ||
| Address string `json:"address"` | ||
| VotingPower uint64 `json:"voting_power,string"` | ||
| } `json:"validator_info"` | ||
| NodeInfo struct { | ||
| Network string `json:"network"` | ||
| } `json:"node_info"` | ||
| SyncInfo struct { | ||
| LatestBlockTime string `json:"latest_block_time"` | ||
| LatestBlockHeight int64 `json:"latest_block_height,string"` | ||
| } `json:"sync_info"` | ||
| } |
There was a problem hiding this comment.
0/5 i preferred the previous struct layout more because it more closely resembled to what we had in the tmrpc
There was a problem hiding this comment.
with the Result wrapper? i removed it because it "leaks" to the caller
i was even thinking of keeping the return value of Status() the same, and casting from this StatusResponse to tm.StatusResponse, so that when we can delete this code, no callers change
There was a problem hiding this comment.
would the cast work? i think it's very strict, also when it comes to struct tags. if not you can just write a converter function
There was a problem hiding this comment.
right, i meant a manual "cast"
There was a problem hiding this comment.
also the converter function may be a little bit more sophisticated to maybe throw errors if we upgrade tendermint lib and it stops working
internal/pkg/cosmos/client.go
Outdated
| @@ -173,14 +169,14 @@ type StatusResponse struct { | |||
| Result struct { | |||
| ValidatorInfo struct { | |||
| Address string `json:"address"` | |||
| VotingPower string `json:"voting_power"` | |||
| VotingPower uint64 `json:"voting_power,string"` | |||
There was a problem hiding this comment.
this is awesome, thank you
| logger.Infof("No EnvPrefix found in config, fetching NodeInfo from GRPC") | ||
| d.nodeInfo, err = d.cosmosClient.NodeInfo(ctx) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to get node info") | ||
| } | ||
| logger.Infof("Connected to the %s node ID: %s", d.nodeInfo.ApplicationVersion.Name, d.nodeInfo.DefaultNodeInfo.DefaultNodeID) | ||
|
|
||
| // if the env prefix is not set, we set it to <APP_NAME>_ (e.g "GAIAD_") | ||
| if cfg.Compose.EnvPrefix == "" { | ||
| cfg.Compose.EnvPrefix = strings.ToUpper(d.nodeInfo.ApplicationVersion.AppName) + "_" | ||
| } |
There was a problem hiding this comment.
i think there were some exceptions to the appname corresponding to the env prefix? onomy?? im not 100% sure, but even it that was the case it's okay for me
There was a problem hiding this comment.
this code didn't change -- just got indented; maybe it was always wrong?
as i see it: if left blank, take it from grpc. if the grpc value is not useful, provide it in the config.
|
blazar now works with
though you need both this patch and to disable |
Need to clean up hacks & verify tests, but in practice it's working