add function to get blockchain id from warp precompile #2633
Conversation
felipemadero
requested review from
sukantoraymond,
meaghanfitzgerald and
cwend-ava
pkg/precompiles/warp.go
Outdated
| if err != nil { | ||
| return ids.Empty, err | ||
| } | ||
| received, b := out[0].([32]byte) |
There was a problem hiding this comment.
Do we need to validate the out array here before we check the 0 index? Also, is the type conversion safe to call or do we need to catch any potential errors from it?
There was a problem hiding this comment.
For the second question, I could be rusty on Go. More so, I am looking to confirm that this conversion fails, if it'll throw an uncaught error
There was a problem hiding this comment.
First question. We are counting that the underlying implementation, if does not error, returns out
with the appropriate size. This is the current semantic.
That say, better to check for len!
There was a problem hiding this comment.
Second question. The type conversion is safe if you also get the b boolean. I mean, there is no uncaught error.
If you do
received := out[0].([32]byte)
you can have uncaught error.
If you do
received, b := out[0].([32]byte)
you will have b==false in conversion error
pkg/precompiles/warp.go
Outdated
| return ids.Empty, err | ||
| } | ||
| received, b := out[0].([32]byte) | ||
| if !b { |
There was a problem hiding this comment.
if the value exists, should prob rename it to ok @felipemadero
There was a problem hiding this comment.
b == type conversion ok
| "github.com/ava-labs/avalanchego/ids" | ||
| ) | ||
|
|
||
| func WarpPrecompileGetBlockchainID( |
There was a problem hiding this comment.
Do we have any sort of framework/etc in place for testing functions like these (ie one's that would require a lot of mocking)?
There was a problem hiding this comment.
we can test this on local network. Do u want to create separate PR for this? @felipemadero
There was a problem hiding this comment.
we don't have framework in place for testing this as unit test. we usually do E2E testing with a local network
deployment, not with a mock. for sure we can consider mocking if we want to really add testing for
all code paths.
There was a problem hiding this comment.
really, how to unit test this stuff, and how to properly tests the commands ux, for example
(prompts, flags paths), needs good design
There was a problem hiding this comment.
open to freely talk on implementation options
There was a problem hiding this comment.
regarding testing this specific feature on E2E, we don't have a command to return blockchain ID,
but it is going to be part of a larger feature in the short term.
#2637
There was a problem hiding this comment.
do we want to add annotations to the functions to point to the specific E2E?
sukantoraymond
force-pushed
the
get-chain-id-from-warp
branch
2 times, most recently
from
001d27a to
8947a39
Compare
Why this should be merged
To avoid the user to input information we cat retrieve from the validators
How this works
How this was tested
How is this documented