Cleanup config #206
Cleanup config #206
Conversation
cam-schultz
requested review from
michaelkaplan13,
minghinmatthewlam,
gwen917,
geoff-vball and
bernard-avalabs
as code owners
| - platform.getHeight | ||
| - platform.validatedBy | ||
| - platform.getValidatorsAt | ||
|
|
||
| `"encrypt-connection": boolean` | ||
| `"info-api-url": string` |
There was a problem hiding this comment.
i'm not too familiar with the info api nodes, is it common for the p chain api node to have info api enabled as well?
There was a problem hiding this comment.
thinking if worth have more documentation about how to find their info api node, or whether to add a comment about testing with the p chain api node
There was a problem hiding this comment.
It's entirely up to the node operator which APIs to support. The P-Chain and Info APIs are independent, so we shouldn't assume that they'll both be enabled on the same node here.
There was a problem hiding this comment.
The public APIs support both the info endpoints and most of the P-chain endpoints (with the exception of getValidatorsAt.
$ curl 'https://api.avax-test.network/ext/info' \
> --header 'Content-Type: application/json' \
> --data '{
> "jsonrpc":"2.0",
> "id" :1,
> "method" :"info.getNetworkID"
> }'
{"jsonrpc":"2.0","result":{"networkID":"5"},"id":1}
I probably would have gone the route of having a single avalancheAPIURL configuration option and adding the path extensions as needed in the application, but not opposed to having these split up in the event that the node used for the P-chain API (possibly private node) doesn't have the Info API enabled (all public APIs have the Info API available).
There was a problem hiding this comment.
Yeah, in the case of the public API, both the info and P-chain API share the same base URL. That's not true in general though, so I think it's better to make as few assumptions here as we can.
There was a problem hiding this comment.
Yeah, totally agree. Thanks for elaborating.
There was a problem hiding this comment.
is there any additional comment/documentation we can add for finding the info api, or is this relatively understood/straightforward for node operators?
There was a problem hiding this comment.
I don't think this is the place to dive too deeply into API availability. We can add a note that the public API should support the required methods though.
Rename subnet -> blockchain in Config
cam-schultz
dismissed stale reviews from minghinmatthewlam, michaelkaplan13, and geoff-vball
via
6251ac2
peers/app_request_network.go
Outdated
| if networkID != constants.MainnetID && | ||
| networkID != constants.FujiID && | ||
| len(APINodeURL) == 0 { | ||
| InfoAPINodeURL == "" { |
There was a problem hiding this comment.
I think InfoAPINodeURL being empty would cause an error above when it's used to get the network ID, so this case isn't possible. Do you know why it was required originally?
There was a problem hiding this comment.
Good catch, we will require this url be non-empty in all cases since it's used to fetch the network ID.
Looking at it again, this check is no longer valid. In an older release, the Avalanchego test network had a set of hard-coded node IDs for Fuji and mainnet, so we didn't need to explicitly connect to them here. That's no longer the case, so we need the info API to fetch the peers in all cases.
| @@ -19,8 +19,13 @@ source "$RELAYER_PATH"/scripts/versions.sh | |||
| full_commit_hash="$(git --git-dir="$RELAYER_PATH/.git" rev-parse HEAD)" | |||
| commit_hash="${full_commit_hash::8}" | |||
|
|
|||
| ./scripts/build.sh | |||
There was a problem hiding this comment.
Was this broken previously? Seems unrelated to the rest of this PR
There was a problem hiding this comment.
Sorry meant to add a comment here explaining this. We removed the build stage from the Dockerfile here, in favor of goreleaser's approach which builds the binary outside of the image. This change should have been included in that PR.
| @@ -145,7 +145,9 @@ func (r *messageRelayer) relayMessage(unsignedMessage *avalancheWarp.UnsignedMes | |||
| // will need to be accounted for here. | |||
| func (r *messageRelayer) createSignedMessage() (*avalancheWarp.Message, error) { | |||
| r.relayer.logger.Info("Fetching aggregate signature from the source chain validators via API") | |||
| warpClient, err := warpBackend.NewClient(r.relayer.apiNodeURI, r.relayer.sourceBlockchainID.String()) | |||
| // TODO: To properly support this, we should provide a dedicated Warp API endpoint in the config | |||
There was a problem hiding this comment.
Can you create an issue for this if it doesn't already exist?
There was a problem hiding this comment.
Yup, this is already captured here: #119
Why this should be merged
Simplifies the relayer configuration by making the following changes:
network-idfield, instead fetching it from the info API.info-api-urlfield instead of reusingp-chain-api-url. This removes the requirement that same node has both APIs enabled.How this works
How this was tested
CI
How is this documented
Updated README