Proposervm API #4029
Proposervm API #4029
Conversation
| // Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. | ||
| // See the file LICENSE for licensing terms. |
There was a problem hiding this comment.
The lint job did not like including 2025 in the copyright date, nor did I find any instances of 2025 in the codebase, so I assume it is alright to add this file with a terminal date of 2024.
|
|
||
| // GetProposerBlockArgs is the parameters supplied to the GetProposerBlockWrapper API | ||
| type GetProposerBlockArgs struct { | ||
| ProposerID ids.ID `json:"proposerID"` |
There was a problem hiding this comment.
I added this type rather than using the existing api.GetBlockArgs to eliminate confusion over what exactly this parameter is. It is the "Proposer ID" listed in the subnets explorer, e.g. https://subnets.avax.network/c-chain/block/64357262, not the block hash/ID as seen by the inner VM.
That said, I'm not sure what the best way to actually source the "Proposer ID" in the general case, since it's not exposed by the innerVM. Similarly, an endpoint to map Block hash -> Proposer ID seems like it would break encapsulation.
There was a problem hiding this comment.
The word Proposer sounds like the actual proposer node. But if that's the common usage, I think it's fine. Maybe ProposerBlockID etc can make it cleaner. I think the actual word for that is OuterBlockID but sounds even more complicated.
There was a problem hiding this comment.
The explorer uses Proposer ID/Proposer Node ID to disambiguate, but yes I agree the naming is awkward. Happy to go with whatever the consensus here is.
cam-schultz
requested review from
meaghanfitzgerald and
StephenButtolph
as code owners
|
|
||
| return handlers, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
what about a unit test that invokes this API?
| server := rpc.NewServer() | ||
| server.RegisterCodec(json.NewCodec(), "application/json") | ||
| server.RegisterCodec(json.NewCodec(), "application/json;charset=UTF-8") | ||
| err = server.RegisterService(&ProposerAPI{vm}, "proposervm") |
There was a problem hiding this comment.
Why don't we have these two also similar to the platformVM?
server.RegisterInterceptFunc(vm.metrics.InterceptRequest)
server.RegisterAfterFunc(vm.metrics.AfterRequest)
Any thoughts about this?
| "github.com/ava-labs/avalanchego/ids" | ||
| "github.com/ava-labs/avalanchego/utils/formatting" | ||
| "github.com/ava-labs/avalanchego/utils/rpc" | ||
| ) |
There was a problem hiding this comment.
Can we add unit tests for this file?
| avajson "github.com/ava-labs/avalanchego/utils/json" | ||
| ) | ||
|
|
||
| type ProposerAPI struct { |
There was a problem hiding this comment.
What is the reason for having the proposerVM as a field instead of just adding the functions to the proposerVM VM?
If we wish to isolate the VM object then it makes sense to add two functions or (an) interface(s) with:
- getLastAcceptedHeight
- GetBlock
and then it'll be easier to test.
|
|
||
| var _ Client = (*client)(nil) | ||
|
|
||
| type Client interface { |
There was a problem hiding this comment.
who/what is consuming this interface? Usually in Golang, interfaces are declared where they're used, not where they're implemented.
| @@ -0,0 +1,70 @@ | |||
| // Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. | |||
There was a problem hiding this comment.
can we add tests to the code in this file?
| p.vm.ctx.Lock.Lock() | ||
| defer p.vm.ctx.Lock.Unlock() | ||
|
|
||
| block, err := p.vm.GetBlock(r.Context(), args.ProposerID) |
There was a problem hiding this comment.
am I reading this wrong and we're actually sending back the entire proposerVM block which also contains the inner VM block? If so, is it possible to only distill the proposerVM bytes?
Why this should be merged
Adds a path
/proposervmto all ProposerVM chains (C, P, subnet-evm) that exposes the following methods:proposervm.getProposedHeightproposervm.getProposerBlockWrapperThis API will also be useful for exposing P-Chain epoch views if ACP-181 is adopted.
How this works
Overrides
CreateHandlersinproposervm.VMto call the inner VM'sCreateHandlersmethod, and register the/proposervmendpoint.How this was tested
Manual testing on a local network. Meaningful unit tests would likely require refactoring
package platformvmto expose internal structs that are used to setup a concrete VM. Happy to take suggestions on alternatives.Need to be documented in RELEASES.md?
Yes