added eth-provider files for pull request

[bzol]

This is the pull request related to the eth-provider grant: https://urbit.org/grants/eth-provider.
Information on how to use the test files and other notes can be found in the README.md file at my original repo: https://github.com/bzol/eth-provider

[pkova]

Okay ladies and gentlemen, it is finally time to start the process to get this merged into %base. For whoever it may concern, these are the preliminary questions I have:

1. We decided to create a separate /lib/eth-provider that mimics the API of /lib/ethio instead of modifying /lib/ethio directly. Is this sane?
2. There's a lot of repetition in this codebase when dispatching to ethio. Is there a way to do some wet gate magic instead?
3. Is the general approach (outlined in the grant document) reasonable?

[zalberico]

@belisarius222 assigning this one to you for review to put it on your radar

[zalberico]

@belisarius222 ping on this for review and/or next steps.

[belisarius222]

I don't know that much about these parts of the system. @Fang- would you be able to review this?

[Fang-]

Let's start with some broad, high-level feedback:

We decided to create a separate /lib/eth-provider that mimics the API of /lib/ethio instead of modifying /lib/ethio directly. Is this sane?

This description made it sound like the new lib would be a more-or-less drop-in replacement for /lib/ethio. Maybe dropping the url=@t from all the samples, but otherwise the same arms with the same signatures. I'd say that's sane and reasonable. But turns out that's not what this is!

Not a fan of encoding the different functionalities provided by /lib/ethio in a $% like $ethin. Would much rather see a /lib/eth-provider that looks structurally like /lib/ethio, and just replaces the function bodies with essentially lines 13-30 of the current /lib/eth-provider.

I guess the underlying reasoning for the current approach is that we still need to send some noun over the wire if we depend on a provider ship, so might as well write an $ethin and $ethout. But $request:rpc:ethereum seems like a much better fit for this. I think it's preferable to only ask providers for formal noun-encoded requests, as opposed to sending them arbitrary rpc json.
Might be a good time to flesh out $response:rpc:ethereum as well, in place of $ethout. Unsure if resolving this long-standing todo would be considered scope creep on the grant though... (May or may not need to be $-([request json] response), in any case.)

urbit/pkg/base-dev/lib/ethereum.hoon

Line 862 in dac240b

::TODO ++ parse-response |= json ^- response

Also, some of the code here is laid out exceptionally dense, and would be made more readable with better indentation and more breathing room.

[jalehman]

@bzol @pkova What's the latest on this? Seems that the ball is in your court.

[bzol]

I've updated the eth-provider, now you can call eth-provider just like ethio (just drop the url). I have some tests for eth-provider at , one for testing the eth-provider itself, the other for testing the integration with the codebase, might be useful for making sure the eth-provider works as intended.

[pkova]

Ok @Fang- I'd say that this is significantly more sane and reasonable since you last took a look. Incorporated all of your feedback.

[jalehman]

@pkova is going to investigate why this has 14,000 additions across 107 files.

[bzol]

Ok, fixed the problem, replaced the files.

[belisarius222]

@Fang- this is ready, please take another look

[Fang-]

This is a lot better already, thank you for your changes so far!

I do still have some comments on the shape by which requests get sent to the provider, and how the provider handles those. The two main points here are around $request:rpc:ethereum vs ethio, and around uniqueness of request identifiers.

Outside of that, the provider's state can be clarified a bit, but this feels directionally correct now. I also left some style comments in specific places, but the advice applies more broadly than just the diffs I highlighted.

[Fang-]

Given that both /sur/eth-watcher, /lib/ethio, and this implement this same type, might be time to move it into /lib/ethereum or maybe +ethereum-types, and have them all import from there instead.

[Fang-]

Any particular reason this matches the arms in /ted/ethio instead of the $request:rpc type from /lib/ethereum?

They're pretty close and don't differ that much, but iirc the latter sticks as close as it can to the official Ethereum RPC API, whereas the former is more focused around urbit-side implementation details. If possible, I think it would be preferable to have this be something along the lines of:

+$  ethin  (list [id=(unit @t) req=request:rpc:ethereum])

With maybe extra flags or w/e for if you want it to be handled as "strict" or "loose" in the case of multiple requests. (Though imo over the network the provider should just always give you all the responses it can get, you can decide on "tightness" locally.)

[bzol]

Now the eth-provider is a drop-in replacement to ethio. The ethio requests in threads are basically forwarded as they are to the provider, but if we go with the $request:rpc type we would need to convert the ethio requests to $request:rpc and back. I think it might make sense to use $request:rpc if people would want to send requests not in the ethio format, but otherwise it is unnecessary to change this. (If we do want to switch to $request:rpc, we would also need to modify or duplicate lib/ethio to support the new eth-provider).

[Fang-]

This probably shouldn't be called a tid for "thread-id", and certainly shouldn't be used as such by /app/eth-provider. If two ships both decide to construct their request identifiers as sequential numbers, they run a serious risk of conflicting. I propose rid for "request-id", but anything in that direction would be fine.

[Fang-]

As mentioned in the comment on the /sur/eth-provider file, we shouldn't be reusing the bare request-id for the thread-id here.

It may be appropriate to concatenate it with the requester's ship name (or provide the ship name as the "parent thread id"?), and use that for the thread id instead. Provides some legibility, and prevents the client from submitting multiple requests with the same id (spider would crash), without any extra implementation effort on our part.

[Fang-]

And similar to that point, if the final $ethin structure we end up settling on ends up including a request id that is meant to get sent to the Ethereum node, we'll probably want to prefix that with the requester's @p, just to make sure that id, too, is actually unique.

[Fang-]

Right now, if I can guess what request/thread IDs you're using, and we both have access to the same provider, I can snoop on the responses to your requests, and infer your requests from that.

I don't think it's a big leak by any means, but it would be nice if it was shielded against. Additionally, "responses" might make more sense than "updates". With that in mind, consider:

/responses/[ship]/[request-id]

Where ship must equal src.bowl for the watch to be successful, and request-id is just whatever identifier that ship will be providing in their %provide pokes.

[Fang-]

Big ?- blocks like these would be easier to read with better indentation and some spacing:

Suggested change

	?- -.arg
	%request-rpc
	;< out=json bind:m (request-rpc:ethio url +.arg)
	(pure:m [%request-rpc out])
	%request-batch-rpc-strict
	;< out=(list [id=@t =json]) bind:m
	(request-batch-rpc-strict:ethio url +.arg)
	(pure:m [%request-batch-rpc-strict out])
	%request-batch-rpc-loose
	?- -.arg
	%request-rpc
	;< out=json bind:m (request-rpc:ethio url +.arg)
	(pure:m [%request-rpc out])
	::
	%request-batch-rpc-strict
	;< out=(list [id=@t =json]) bind:m
	(request-batch-rpc-strict:ethio url +.arg)
	(pure:m [%request-batch-rpc-strict out])
	::
	%request-batch-rpc-loose
	etc...

[Fang-]

You don't need to keep the first line of n-ary runes clear like this. But for bigger blocks among one-liners, I would add some spacers:

Suggested change

	[%get-logs-by-hash =hash:block contracts=(list address) =topics]
	$:
	%get-logs-by-range
	contracts=(list address)
	=topics
	=from=number:block
	=to=number:block
	==
	[%get-next-nonce =address]
	[%get-balance =address]
	==
	+$ ethout
	$%
	[%request-rpc res=json]
	[%request-batch-rpc-strict res=(list [id=@t =json])]
	[%get-logs-by-hash =hash:block contracts=(list address) =topics]
	::
	$: %get-logs-by-range
	contracts=(list address)
	=topics
	=from=number:block
	=to=number:block
	==
	::
	[%get-next-nonce =address]
	[%get-balance =address]
	==
	+$ ethout
	$% [%request-rpc res=json]
	[%request-batch-rpc-strict res=(list [id=@t =json])]

[Fang-]

I think this (and maybe some of the other logic here) is already implemented in /lib/eth-provider, so can de deduplicated here.

[Fang-]

Talked briefly with @philipcmonk about this. We'll probably want to revert the changes to the roller and eth-watcher threads here, because those have an important edge case to deal with: what if your provider breaches?

We can just let them keep doing their static-url thing for now, I wouldn't consider accounting for that case in scope for this PR.

[jalehman]

@bzol any updates?

[bzol]

@Fang I modified eth-provider according to your suggestions, except this issue: #5909 (comment)

[bzol]

Okay, I've changed the provider mode to accept (list response:rpc), we were wondering with @pkova how should we handle the provider being offline/not responding (timeout maybe?). We also added entropy to the request-id (rid) in lib/eth-provider, because some arms like get-latest-block make two separate request-batch-rpc-loose calls in the same thread, so the wire would not be unique when subscribing to a provider. For some strange reason the src.bowl in the second request-batch-rpc-loose call is the @p of the provider, not the client's @p, do you have any idea why that is?

[Fang-]

The $ethin:eth-provider->$request:rpc:ethereum change from the latest commit is good, but it's not clear to me why the response/output type was changed into a (list $response:json-rpc). The $ethout type was already pretty close to $response:rpc:ethereum, which is what I think we really want here.

From my first review:

I guess the underlying reasoning for the current approach is that we still need to send some noun over the wire if we depend on a provider ship, so might as well write an $ethin and $ethout. But $request:rpc:ethereum seems like a much better fit for this. I think it's preferable to only ask providers for formal noun-encoded requests, as opposed to sending them arbitrary rpc json.
Might be a good time to flesh out $response:rpc:ethereum as well, in place of $ethout. Unsure if resolving this long-standing todo would be considered scope creep on the grant though... (May or may not need to be $-([request json] response), in any case.)

Given that the old $ethout:eth-provider is just an expanded $response:rpc:ethereum, you can probably slot that right into the /lib/ethereum, and keep using it like you used to.

To reiterate from our last call, I think the most straightforward API here is one where you give the /app/eth-provider a (list [id=(unit @t) request:rpc:ethereum]), and receive a (list [id=(unit @t) response:rpc:ethereum]) (of equal or lesser length) in return.

how should we handle the provider being offline/not responding (timeout maybe?).

Yes a timeout seems right. In theory they'll "resolve eventually" but that might not be very nice developer ergonomics. For the ask-over-HTTP case I think Iris times out requests eventually, so would make sense to match that here.

We also added entropy to the request-id (rid) in lib/eth-provider, because some arms like get-latest-block make two separate request-batch-rpc-loose calls in the same thread, so the wire would not be unique when subscribing to a provider.

There's another option here, where you only open a single subscription per thread and just receive multiple facts on it. Might require you to check "did we open a subscription for this thread yet" everywhere, but shouldn't be hard to factor that out. I'm not requiring this, but it might be a nicer solution.

For some strange reason the src.bowl in the second request-batch-rpc-loose call is the @p of the provider, not the client's @p, do you have any idea why that is?

If you receive a %fact, the src.bowl is going to be set to the publisher that gave you that fact. Threads may "pause" their execution to wait on some event, and when they "resume" the bowl might have changed to reflect that event (its source, current timestamp, new entropy, etc.).

[jalehman]

@pkova and @bzol met yesterday and are working through an outstanding TODO that, while not directly related, might as well go into this PR. Should be ready to review then.

[bzol]

This is ready for review again, we changed the types so now the eth-provider uses both request and response:rpc:ethereum, and added a 10 second timeout that makes the lib/eth-provider thread fail if the provider is not available for some reason.

[jalehman]

Next steps on this:

1. Get @Fang- 's revised opinion on the code as it stands
2. Create a comprehensive testing plan involving live usage that demonstrates that this will not cause any regressions

[Fang-]

Alright, we're getting close. I think this is looking good for the msot part. Some higher-level comments here, after we resolve this we can do a pass of implementation-level/style comments to clean this up, and then we should be good to go. (:

[Fang-]

I like the addition of the "dirty" type. In most cases, that should just be for internal usage, and not really exposed to the api/library consumer, right?

Feels good to have an expanded $response type, but I think it's preferable to keep the tags matching their requests. Even if multiple requests give a response with the same type, they might not be semantically equivalent. (You already account for this by separating %transaction-count and %balance, for example.) More importantly: it seems like a nice property to be able to know the response type from knowing the request that's being sent. The 1-to-1 relation seems worth preserving, so I'll ask you update this to use the old naming style.

If you're concerned about duplicating the inner types: I don't think it's a particular problem for any in this list. Adding separate types to capture those would probably be overkill.

[Fang-]

logs

Could do it. Unless these use a special one-off format, we should already have types & logic for it, from the "get logs" requests.

contractAddress

What's messy about (unit @ux)? May need to do an existence or null check during parsing, but shouldn't be too difficult. For the latter, stdlib json destructuring already has a helper (+mu:dejs:format).

is this correct

What, the logs-bloom field? I believe that's just some fixed amount of bytes (256 of them), for which @ux is a fine representation, yes.

However:

• block-number and transaction-index should probably be @ud, since they're sequence numbers.
• type should be either 0 or 2, for "transfer" and "contract creation or call" respectively. We probably want to do type=?(%transfer %call) for that.
• status, similarly, is either 0 for failure, or 1 for success. I think it can just be a ? loobean.
• to and from should use the $address type. That's an alias for @ux, so nothing changes, but it's more semantically correct.

[bzol]

Made the changes for this round.