Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🫖 [View Integration Test Results](server error: invalid xunit xml) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🫖 [View Integration Test Results](server error: invalid xunit xml) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🫖 [View Integration Test Results](server error: invalid xunit xml) |
There was a problem hiding this comment.
Nice work! 🎉 I did not yet look at the cadence part (nor do i really have the expertise to, but who does ;))
1
The whole thing about where to import from is pretty sad eg 0x0ae53cb6e3f42a79, 0xee82856bf20e2aa6, ... We have this in ethereum too to some extent but at least it is parameterized by something like address xyzContract. I think this means we really need to think about the deployment process here. Might be worth using a template engine to supply the addresses with some kind of build phase. I'm sure they know this is a problem for anyone building on this system but they just don't have any tooling around it yet :( .
2
This is really a pretty small nit. Broadly speaking, I am of the opinion that flow-fetcher should be as dumb a wrapper as possible so that as we expand functionality from within the OCW we can access whatever we need from this proxy. I think we are pretty close right now but I think this does a little too much re-shaping that is why this is merely a nit. I think ideally we would just re-expose methods from flowClient *client.Client as "RPC" endpoints.
To me, the endpoints should be something like POST /GetBlockByID and POST /GetBlockByHeight with json post bodies that match to the args by name and return values that match exactly to their gRPC counterparts. Go being as it is, mapping the return values exactly may require a decent amount of boilerplate but maybe not. I think the objects generated by protobuf serialize to JSON pretty well if I remember correctly. We should be able to serialize those return values directly to JSON.
One downside that I see here is that we somewhat lose control over the return value shapes so that when we update the version of flow in go.mod our return values may change shape from under our feet and break our api. Pros and cons I suppose. Maybe that is where we include /v1 so we can have backward compatible versions?
What do you think?
3
It would be cool to see some unit tests of the flow-fetcher. It could be done with mocks or with a real flow client. Go unit testing is pretty friendly if not boilerplate heavy, the easiest way to do it if you want to mock things out is to mostly depend on interfaces then mock out the interfaces. Reminds me a lot of Rust unit testing in that way. The dependency injection story in Go is still woefully lacking and mostly done by hand as far as I can tell.
| // Amount float64 `json:"amount"` | ||
| // } | ||
|
|
||
| func getLockEvents(flowClient *client.Client, eventsInfo FlowEventsInfo) ([]FlowEvent, error) { |
There was a problem hiding this comment.
Is this more generic than merely getLockEvents?
| TransactionIndex: lockEvent.TransactionIndex, | ||
| EventIndex: lockEvent.EventIndex, | ||
| Topic: lockEvent.Type, | ||
| Data: fmt.Sprintf("{\"asset\":\"%s\",\"recipient\":\"%s\",\"amount\":%d}", event.Asset(), event.Recipient(), event.Amount()), |
There was a problem hiding this comment.
Possible double encode here? Does this end up being the whole escaped json string inside a json payload type of thing?
| @@ -0,0 +1,34 @@ | |||
| package starport | |||
There was a problem hiding this comment.
Generally throughout this file should we be using checked access with err return values? What happens when you do evt.Fields[0] but len(evt.Fields) == 0 ? Is that a go panic at that point? Same goes for unchecked casts in this file
| } | ||
|
|
||
| block, err := func() (*flow.Block, error) { | ||
| if blockInfo.Height == 0 { |
There was a problem hiding this comment.
this is an example of where i think we're perhaps being a little too "smart" in this wrapper? again - total nit
This comment has been minimized.
This comment has been minimized.
|
🫖 [View Integration Test Results](server error: invalid xunit xml) |
This comment has been minimized.
This comment has been minimized.
toninorair
changed the title
toninorair
changed the title
Add Flow support to Gateway
toninorair
changed the title
|
🫖 [View Integration Test Results](server error: invalid xunit xml) |
|
🫖 [View Integration Test Results](server error: invalid xunit xml) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
🫖 [View Integration Test Results](server error: invalid xunit xml) |
3 participants
Adding support for the Flow blockchain. The following PR includes:
The following PR DOES NOT INCLUDE:
Integration of flow client to Gateway Cash Pallet and scenario tests (coming in the next PR).