-
Notifications
You must be signed in to change notification settings - Fork 24
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
Config Service bugfixes, Admin API and Gateway Location RPC #394
Conversation
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.
A lot going on in here but on an initial pass lgtm.
One observation is that we dont appear to have any tests for the config service which may bite at the client integration phase
|
||
let key_type = KeyType::from_i32(request.key_type) | ||
.map_err(|_| Status::invalid_argument("invalid key type supplied"))?; | ||
let pubkey = self |
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 this check not redundant, in that if the pub key is invalid wont it already be caught at the sig verification on line 75?
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.
Good question but these are checking different keys/things.
They function on line 75 is checking the request to add the new key was signed by an already-registered key (the default key in the db or any admin keys saved to the db) so it would fail if it attempted to validate the request with the key being submitted to the add_key
method.
this check validates the key you’re trying to register is a valid key worth saving and also converts the bytes to a PublicKey in order to do the follow up check that it’s a valid key for the network this config instance is servicing (mainnet vs devnet)
8396216
to
418c2be
Compare
I agree with you; looking forward to wrapping all of my DB accessors in a trait i can test from outside the rpc methods |
) -> GrpcResult<GatewayLocationResV1> { | ||
unimplemented!() | ||
// Should this rpc be admin-authorized only or should a requesting pubkey |
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.
My understanding right now is only HPR's will be requesting gateway locations.
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.
@madninja and i are discussing whether or not this needs to become a more generic metadata
method for retrieving any on-chain data about a hotspot, inclusive of the asserted location but that would be either primarily or exclusively for the other oracles so between that and the HPRs still under the umbrella of "network infrastructure" so likely authenticated but we don't currently authenticate any requests the oracles make (they only request from their host-local blockchain-node instance) so request signing would be a new requirement for them
418c2be
to
29bc72d
Compare
d751c46
to
42ecae9
Compare
42ecae9
to
3c82891
Compare
! Depends on helium/proto#288
This doozy introduces several bug fixes in the implementation of the streaming RPCs, retrieving records from the DB and decoding their types, integer size changes in the upstream proto dependency, and devaddr constraints records