-
Notifications
You must be signed in to change notification settings - Fork 12
feat(cli): add espresso-reader #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(cli): add espresso-reader #150
Conversation
🦋 Changeset detectedLatest commit: 79f7ac7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage Report for ./apps/cli
File CoverageNo changed files found. |
b4e999f to
78b5056
Compare
0a1e320 to
4bf2c98
Compare
|
Instructions fixing typos :) |
miltonjonat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't access the espresso endpoint on http://localhost:8080/espresso, only directly on http://localhost:8770. Is that expected?
It's missing some instructions. We expose 4 espresso API endpoints via names PATHs, they are:
So if you wanna call the
|
| DATABASE_URL: postgres://postgres:password@database:5432/espresso_reader | ||
| ESPRESSO_BASE_URL: http://espresso:8770 | ||
| ESPRESSO_NAMESPACE: 51025 | ||
| ESPRESSO_STARTING_BLOCK: 101 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rollups-espresso-reader does a weird thing: it publishes two endpoints /nonce and /submit on a port configurable with env ESPRESSO_SERVICE_ENDPOINT (default I think is 8080). We'd need to expose these too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this format?
/espresso/reader/nonce/espresso/reader/submit
If the developer interaction if gonna be focused on those endpoints, we could even hide everything from espresso-dev-node and only publish those at /espresso/submit and /espresso/nonce, also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miltonjonat I sent a fixup where you can access those endpoints at:
http://localhost:8080/espresso/reader/noncehttp://localhost:8080/espresso/reader/submit
Please, let me know if it works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, these endpoints are currently conveniences for the Espresso integration, and strictly speaking they could (or should?) be part of another component separate from the reader itself. So, no, I wouldn't use "reader" there.
Frankly, I think it's also quite odd to put it under "espresso" in this context, because that "espresso" there is meant to be the (local) Espresso Network, while these are endpoints on the Node itself.
Tbh, the idea here was also that the Node would provide /nonce and /submit functionalities as something more universal (i.e., to also be used when integrated with Avail, Celestia, etc). And that clients wouldn't even know which alt-DA was being used by the app/node.
TL;DR, I'd use http://localhost:8080/nonce and http://localhost:8080/submit directly; or we could come up with a nice namespace for convenience endpoints of this kind (maybe http://localhost:8080/tx/nonce?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand there is no unified way to send a transaction. anvil expects ethereum transactions as specified by eth_sendTransaction, espresso expects a transaction object, avail has its own spec.
So having a single /nonce and /submit at the root of the service, regardless the configured DA, with various formats will be extremely confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, one path what would be very adherent to the current architecture would be something like L2tx, leading to http://localhost:8080/L2tx/nonce and http://localhost:8080/L2tx/submit (I suggested tx above, but I admit that L2tx would be more precise)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you going to use a single format for DAs with different requirements?
I.e. how do you define the namespace in case of Espresso?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current design, the client/user does not inform any DA-specific parameters in the payload it submits. Whatever is required to submit to the alt-DA configured for the application should be given by DA-specific parameters defined upon deployment of the app, and stored as on-chain app configuration. The Node will fetch that from the application contract (it's currently defined as an ABI encoded method+parameters), and thus know how to handle tx submission when /submit is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming back to this from the discussions on #171, I'm now thinking that a path that would make sense would be http://localhost:8080/eip712/nonce and http://localhost:8080/eip712/submit
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EIP-712 is a typed structured data hashing and signing standard. I don't think it should be the name of the service.
78b5056 to
d2c37a4
Compare
dc4d99a to
fda0e28
Compare
a82afdd to
a7e55c5
Compare
a697bb4 to
933a91d
Compare
a7e55c5 to
7feeb21
Compare
933a91d to
0a9b6e4
Compare
|
I tried it out, had some issues, and couldn't run in the end.
|
44df851 to
1297ea6
Compare
The failure was The migration code doesn't mention the creation of this |
|
That's part of the Node migration, which has to be executed before the Espresso Reader's one. |
Sent a fixup for that. 0fb7ac6 I'll have to rebase this on top of |
39558cf to
d496299
Compare
0fb7ac6 to
544d18b
Compare
|
@endersonmaia cool new instructions! I like the
|
Strange that it is reporting a database url as Did you change any of that? |
I didn't change anything, but AFAIK in Node v2 the database is always called |
We are testing locally with branch |
We define the env var
|
|
Right.. maybe it's not going through, or not being respected by the Node code..? |
|
@miltonjonat my suggestion is that you go to the cartesi/rollups-node local repository at the Then you'll have a Let me know if anything changes. |
b295744 to
48ddef2
Compare
b999336 to
24ad87e
Compare
|
Time to rebase this one. |
f6996c4 to
7e1b164
Compare
ad79708 to
babb7bd
Compare
62cdb80 to
79f7ac7
Compare
You can test this with:
The
rollups-espresso-readerendpoints should be available at: