feat(admin): added rpc method for getL2ToL2 Message(msgHash) #265
Conversation
s4rv4d
changed the title
admin/admin.go
Outdated
| } else { | ||
| m.log.Error("auto relay not enabled", "args", *args) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
the indexer runs regardless of the auto relayer. maybe some thing like "Interop is not enabled, please enable interop with --interop.enabled"
There was a problem hiding this comment.
also this should not be a log it should return an RPC error with the correct code
admin/admin.go
Outdated
| reply := storeEntry.Message() | ||
|
|
||
| return reply |
There was a problem hiding this comment.
nit
return storeEntry.Message()
| @@ -23,7 +23,7 @@ func TestAdminServerBasicFunctionality(t *testing.T) { | |||
| testlog := testlog.Logger(t, log.LevelInfo) | |||
|
|
|||
There was a problem hiding this comment.
can u add a test to see that the result is returned correctly? or alternatively can be in supersim_test
There was a problem hiding this comment.
added to supersim_test
| adminServer := &AdminServer{log: log, port: port, networkConfig: networkConfig} | ||
|
|
||
| if networkConfig.InteropEnabled && indexer != nil { | ||
| adminServer.l2ToL2MsgIndexer = indexer | ||
| } |
There was a problem hiding this comment.
nit- if it's nil then it will be initialized correctly
adminServer := &AdminServer{log: log, port: port, networkConfig: networkConfig, l2ToL2MsgIndexer: indexer}
admin/admin.go
Outdated
| if s.networkConfig.InteropEnabled { | ||
| rpcMethods.l2ToL2MsgIndexer = s.l2ToL2MsgIndexer |
There was a problem hiding this comment.
rpcMethods.l2ToL2MsgIndexer = s.l2ToL2MsgIndexer
no need to check for interopEnabled. from the perspective of the admin server, if it gets passed a non-nil indexer it assumes it's usable
orchestrator/orchestrator.go
Outdated
| func (o *Orchestrator) L2ToL2MessageIndexer() *interop.L2ToL2MessageIndexer { | ||
| return o.l2ToL2MsgIndexer | ||
| } | ||
|
|
There was a problem hiding this comment.
see comment on supersim.go
once the admin server start logic is moved into here, we can remove this fn
admin/admin.go
Outdated
|
|
||
| reply := storeEntry.Message() | ||
|
|
||
| return reply |
There was a problem hiding this comment.
can u verify that this returns correctly (serialized and formatted correctly) when u call the json rpc server? and add a test for it?
There was a problem hiding this comment.
did test it on local
will add test cases too
admin/admin.go
Outdated
|
|
||
| if m.networkConfig.InteropEnabled && m.l2ToL2MsgIndexer != nil { | ||
|
|
||
| storeEntry, err := m.l2ToL2MsgIndexer.Get(*args) |
There was a problem hiding this comment.
also need to handle case if the storeEntry just doesn't exist - should end up returning nil
| m.log.Error("valid msg hash not provided", "args", *args) | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: early return is better than the if/else statement below
if m.l2ToL2MsgIndexer == nil {
m.log.Error("auto relay not enabled", "args", *args)
return nil
}
^ we should return a better error
admin/admin.go
Outdated
| if args == nil { | ||
| m.log.Error("valid msg hash not provided", "args", *args) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
can we validate that it's a correct msgHash?
|
adding a screenshot of test results on local 
|
|
Updated example rpc response
|
| type JSONL2ToL2Message struct { | ||
| Destination uint64 `json:"Destination"` | ||
| Source uint64 `json:"Source"` | ||
| Nonce *big.Int `json:"Nonce"` | ||
| Sender common.Address `json:"Sender"` | ||
| Target common.Address `json:"Target"` | ||
| Message hexutil.Bytes `json:"Message"` | ||
| } |
There was a problem hiding this comment.
nit: you can prob just import this from admin.JSONL2ToL2Message
In a future PR might make sense to unify with the indexer's message type so we don't have two types. As discussed, out of scope for this PR! just noting.
Description
added getL2ToL2 Message(msgHash), essentially letting devs fetch L2ToL2Message via msgHash
Metadata