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

API v2 specification along with the new requirements #300

Closed
wants to merge 43 commits into from
Closed

Conversation

kacpersaw
Copy link
Contributor

No description provided.

@kacpersaw kacpersaw linked an issue Jan 24, 2024 that may be closed by this pull request
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.

great start. see comments.

one more thing to add: TransactionService.EstimateGas which receives a tx (with max gas unset) and returns a recommended max gas.

spacemesh/v2/account.proto Outdated Show resolved Hide resolved
spacemesh/v2/account.proto Outdated Show resolved Hide resolved
spacemesh/v2/account.proto Outdated Show resolved Hide resolved
spacemesh/v2/block.proto Outdated Show resolved Hide resolved
spacemesh/v2/block.proto Outdated Show resolved Hide resolved
spacemesh/v2/tx.proto Outdated Show resolved Hide resolved
spacemesh/v2/tx.proto Outdated Show resolved Hide resolved
spacemesh/v2/tx.proto Outdated Show resolved Hide resolved
spacemesh/v2/tx.proto Outdated Show resolved Hide resolved

// An immutable Spacemesh transaction.
// do not include mutable data such as tx state or result.
message TransactionV1 {
Copy link
Member

Choose a reason for hiding this comment

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

need to also add recipient and send amount (for Send txs). I'm not 100% sure how to structure this. we should probably add a subfield, something liked ParsedContents, that's oneof depending on the type of tx (spawn, self spawn, send, etc.)

Copy link
Contributor Author

@kacpersaw kacpersaw Feb 5, 2024

Choose a reason for hiding this comment

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

we can also left that to user. In explorer I'm parsing every tx from raw format, but if you want to put everything as fields we can do that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is an important requirement for consumers. We shouldn't require them to parse a raw TX, the API should provide the parsed TX. If for some reason we think this will be expensive, we can add a flag to the Request object to enable this.

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.

Great work, design is almost done.

spacemesh/v2/network.proto Outdated Show resolved Hide resolved
uint32 layer_duration = 3; // layer duration, in seconds
bytes genesis_id = 4;
string hrp = 5;
uint32 first_genesis = 7; // first effective genesis layer
Copy link
Member

Choose a reason for hiding this comment

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

I see in the code that this has to do with checkpoint restore. In practice effective genesis and "first genesis" are the same. Let's keep things simple and collapse these two into a single data item called effective_genesis_layer for clarity.

spacemesh/v2/network.proto Outdated Show resolved Hide resolved
spacemesh/v2/network.proto Outdated Show resolved Hide resolved
uint32 synced_layer = 3; // the last layer node has synced
uint32 verified_layer = 4; // the last layer node has verified
uint32 head_layer = 5; // head layer is the tip
uint32 head_block = 6;
Copy link
Member

Choose a reason for hiding this comment

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

I think we're missing one. Here's my proposal:

  uint32 synced_layer = 3; // last layer node has synced
  uint32 verified_layer = 4; // last layer node has verified
  uint32 head_layer = 5; // current chain head; the last layer the node has gossiped or seen
  uint32 current_layer = 6; // current layer, based on clock time

synced_layer is only useful while syncing. Blocks are also received via gossip. The relationship should be:

synced_layer <= verified_layer <= head_layer <= current_layer


// An immutable Spacemesh transaction.
// do not include mutable data such as tx state or result.
message TransactionV1 {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is an important requirement for consumers. We shouldn't require them to parse a raw TX, the API should provide the parsed TX. If for some reason we think this will be expensive, we can add a flag to the Request object to enable this.

string template = 3;
uint32 method = 4; // this is actually limited by uint8, but no type for that.
Nonce nonce = 5;
LayerLimits limits = 6;
Copy link
Member

Choose a reason for hiding this comment

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

confirmed, layer limits for tx are not currently enabled. let's remove this for now for simplicity and to avoid confusion.

@lrettig
Copy link
Member

lrettig commented Feb 15, 2024

See latest commit, this is my attempt to add parsed tx contents. Feel free to tweak!

kacpersaw and others added 5 commits February 16, 2024 08:51
Co-authored-by: Lane Rettig <lanerettig@gmail.com>
Co-authored-by: Lane Rettig <lanerettig@gmail.com>
@kacpersaw
Copy link
Contributor Author

See latest commit, this is my attempt to add parsed tx contents. Feel free to tweak!

looks good, we can start with that

@kacpersaw kacpersaw marked this pull request as ready for review February 16, 2024 23:49
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.

lgtm! just need one more small change.

spacemesh/v2alpha1/node.proto Outdated Show resolved Hide resolved
repeated string pubkey = 2;
}

message TransactionContents {
Copy link
Member

Choose a reason for hiding this comment

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

This should also be versioned, but it lives inside a versioned tx type so maybe that's enough. What do you think?

uint64 gas_price = 7; // fee per unit gas, in smidge
uint64 max_spend = 8;
bytes raw = 9;
enum TransactionType {
Copy link
Member

Choose a reason for hiding this comment

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

For the record there are other types, involving the vault and vesting wallet templates, but neither is relevant right now and we can add these later.

kacpersaw and others added 21 commits February 20, 2024 08:49
Bumps google.golang.org/protobuf from 1.31.0 to 1.32.0.

---
updated-dependencies:
- dependency-name: google.golang.org/protobuf
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/grpc-ecosystem/grpc-gateway/v2](https://github.com/grpc-ecosystem/grpc-gateway) from 2.18.1 to 2.19.0.
- [Release notes](https://github.com/grpc-ecosystem/grpc-gateway/releases)
- [Changelog](https://github.com/grpc-ecosystem/grpc-gateway/blob/main/.goreleaser.yml)
- [Commits](grpc-ecosystem/grpc-gateway@v2.18.1...v2.19.0)

---
updated-dependencies:
- dependency-name: github.com/grpc-ecosystem/grpc-gateway/v2
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.60.1 to 1.61.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.60.1...v1.61.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [github.com/grpc-ecosystem/grpc-gateway/v2](https://github.com/grpc-ecosystem/grpc-gateway) from 2.19.0 to 2.19.1.
- [Release notes](https://github.com/grpc-ecosystem/grpc-gateway/releases)
- [Changelog](https://github.com/grpc-ecosystem/grpc-gateway/blob/main/.goreleaser.yml)
- [Commits](grpc-ecosystem/grpc-gateway@v2.19.0...v2.19.1)

---
updated-dependencies:
- dependency-name: github.com/grpc-ecosystem/grpc-gateway/v2
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Bartosz Różański <bartek.roza@gmail.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.61.0 to 1.61.1.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.61.0...v1.61.1)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
@kacpersaw
Copy link
Contributor Author

kacpersaw commented Apr 2, 2024

Specification was divided into separate PRs to avoid merging all definitions at once. Current progress can be found here: #295 (comment)

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.

Finalize API v2 specification along with the new requirements
5 participants