Skip to content
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

v2alpha1: Add LayerService with transaction and block definition #319

Merged
merged 16 commits into from
Apr 29, 2024

Conversation

kacpersaw
Copy link
Contributor

@kacpersaw kacpersaw commented Mar 20, 2024

API v2 spec #300

Add new layer service for v2alpha1 based on API v2 spec.



service LayerService {
rpc Get(LayerRequest) returns (Layer);
Copy link
Contributor

@dshulyak dshulyak Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggest to add proper List/Stream api like you did for several other services and drop this not so useful Get

message BlockV1 {
bytes id = 1; // block hash
repeated Transaction transactions = 2; // block transactions, in canonical order
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is also not so useful, would be better to report block hash and provide List/Stream apis for rewards and transactions

Copy link
Member

@lrettig lrettig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just requesting one small tweak to an enum, otherwise LGTM!

spacemesh/v2alpha1/layer.proto Outdated Show resolved Hide resolved
spacemesh/v2alpha1/layer.proto Outdated Show resolved Hide resolved
@lrettig lrettig mentioned this pull request Apr 10, 2024
spacemesh/v2alpha1/layer.proto Outdated Show resolved Hide resolved
@kacpersaw kacpersaw requested a review from lrettig April 19, 2024 08:13
@kacpersaw kacpersaw requested a review from poszu April 19, 2024 08:13
@lrettig
Copy link
Member

lrettig commented Apr 22, 2024

@kacpersaw just a couple of clarifications from #287:

Can you confirm that we intend API v2 to only return the canonical block for a layer, same as API v1? (#287 (comment))

How do we intend the API to reveal tx status and validity? Is the plan to add this later? (#287 (comment))

How do we intend the API to report reorg events?

Thanks.

@kacpersaw
Copy link
Contributor Author

@lrettig

Can you confirm that we intend API v2 to only return the canonical block for a layer, same as API v1? (#287 (comment))

Yes, only applied block will be returned.

How do we intend the API to reveal tx status and validity? Is the plan to add this later? (#287 (comment))

This PR is about LayerService, we can discuss this in TransactionService PR when it'll be created.

How do we intend the API to report reorg events?

How do you want it to look like?

@lrettig
Copy link
Member

lrettig commented Apr 26, 2024

How do we intend the API to report reorg events?

How do you want it to look like?

There was some discussion of this in #38 but it was inconclusive. I think as a next step it would be good to see how APIs for other projects handle this case. For the purposes of this PR let's just do something simple and reasonable; we can continue the discussion in #38 to decide how we want to handle this more correctly and completely (ideally, API v2 should include this).

I don't see any issue with LayerRequest. For LayerStream maybe we just terminate the stream if we detect a reorg and then, for now, require the client to check on its end if there's been a reorg. As for how to do that, it's non trivial since #319 (comment). In other words we still need a quick, easy way for a client to detect if a reorg has occurred.

I don't think this issue should block merging of this PR. Let's sort this out in #38.

@lrettig lrettig mentioned this pull request Apr 26, 2024
@kacpersaw kacpersaw merged commit 491b98a into master Apr 29, 2024
1 check passed
@kacpersaw kacpersaw deleted the v2alpha1-layer branch April 29, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants