-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
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.
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/tx.proto
Outdated
|
||
// An immutable Spacemesh transaction. | ||
// do not include mutable data such as tx state or result. | ||
message TransactionV1 { |
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.
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.)
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.
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.
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.
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.
Co-authored-by: Lane Rettig <[email protected]>
Co-authored-by: Lane Rettig <[email protected]>
Co-authored-by: Lane Rettig <[email protected]>
Co-authored-by: Lane Rettig <[email protected]>
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.
Great work, design is almost done.
spacemesh/v2/network.proto
Outdated
uint32 layer_duration = 3; // layer duration, in seconds | ||
bytes genesis_id = 4; | ||
string hrp = 5; | ||
uint32 first_genesis = 7; // first effective genesis layer |
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 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/node.proto
Outdated
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; |
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 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
spacemesh/v2/tx.proto
Outdated
|
||
// An immutable Spacemesh transaction. | ||
// do not include mutable data such as tx state or result. | ||
message TransactionV1 { |
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.
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.
spacemesh/v2/tx.proto
Outdated
string template = 3; | ||
uint32 method = 4; // this is actually limited by uint8, but no type for that. | ||
Nonce nonce = 5; | ||
LayerLimits limits = 6; |
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.
confirmed, layer limits for tx are not currently enabled. let's remove this for now for simplicity and to avoid confusion.
See latest commit, this is my attempt to add parsed tx contents. Feel free to tweak! |
Co-authored-by: Lane Rettig <[email protected]>
Co-authored-by: Lane Rettig <[email protected]>
looks good, we can start with that |
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.
lgtm! just need one more small change.
repeated string pubkey = 2; | ||
} | ||
|
||
message TransactionContents { |
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.
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 { |
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.
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.
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] <[email protected]>
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] <[email protected]>
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] <[email protected]>
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] <[email protected]>
Co-authored-by: Bartosz Różański <[email protected]>
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] <[email protected]>
Specification was divided into separate PRs to avoid merging all definitions at once. Current progress can be found here: #295 (comment) |
No description provided.