-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleanup config #206
Merged
Merged
Cleanup config #206
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
76aad59
remove network-id from cfg
cam-schultz 50561ea
separate info and p chain api urls
cam-schultz 38609b3
require fully specified endpoints
cam-schultz f742084
fix tests
cam-schultz 7d7789c
removed unused func
cam-schultz 0e54e63
check error
cam-schultz 6884e54
update readme
cam-schultz 2b0017a
key sources+dsts by chain
cam-schultz 6251ac2
Merge pull request #212 from ava-labs/key-blockchain-id
cam-schultz 6c9c54e
Merge branch 'main' into cleanup-cfg
cam-schultz 056833b
note public api suitability
cam-schultz 0a49530
require info api url
cam-schultz f95a7dd
lower case var
cam-schultz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public APIs support both the info endpoints and most of the P-chain endpoints (with the exception of
getValidatorsAt
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, totally agree. Thanks for elaborating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, I agree