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 2.0 #149

Closed
wants to merge 34 commits into from
Closed

API 2.0 #149

wants to merge 34 commits into from

Conversation

avive
Copy link
Member

@avive avive commented Jun 7, 2021

Draft PR to review and discuss GlobalState service changes to support Spacemesh Apps in release 0.3.

@avive avive added the feature label Jun 7, 2021
@avive avive added this to the v1.2 milestone Jun 7, 2021
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/global_state.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/global_state.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/global_state.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
// Information about an app template in the global state store
message Template {
TemplateAddress address = 1; // template address
string name = 2; // template name
Copy link
Member

Choose a reason for hiding this comment

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

See comment in issue, I don't understand why this is required

proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
// but is convenient to return them directly as part of apps' metadata.
message App {
AccountId address = 1; // App's address in global space
string name = 2; // short descriptive name
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is good to have a short app name that describes it. I think that it is something that is set by spwaner and spawn time per app.

AccountId address = 1; // App's address in global space
string name = 2; // short descriptive name
TemplateAddress template_address = 3; // App's template address
LayerNumber spawn_layer = 4; // Layer in which app was spawned
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which field? These are all meta-data about an app which is useful. It is useful to know in which layer an app was deployed to the mesh although this info can be obtained from the spawn transaction - it is just convince to include some metadata that can be obtained from transactions about the app in response to this call because the intent of the caller is to get this data.

repeated Template templates = 2;
}

// A request to get an app's variable by var id
Copy link
Member

Choose a reason for hiding this comment

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

Instead of "variable" can we give these a more descriptive name, e.g., state entry?

Copy link
Member

Choose a reason for hiding this comment

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

@YaronWittenstein said these are called "storage variables" in SVM

// A request to get an app's variable by var id
message GetVariableRequest {
AccountId address = 1; // app's address
LayerNumber layer_number = 2; // requested value as of a layer. Pass MAX_UI32 to get latest value
Copy link
Member

Choose a reason for hiding this comment

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

As noted in the issue, will only work for archival nodes! What do non-archival nodes do if this is specified? Error?

Copy link
Member Author

Choose a reason for hiding this comment

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

See my comment there - full nodes will hold state for n layers back and archive nodes since genesis but from an api level it is good to have this like this and nodes will return not_found status if they can't satisfy this specific request for a layer which is not recent.

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 that works, but for the record, we cannot force full nodes to hold intermediate state for any minimum number of layers since this is extraprotocol

message GetVariableRequest {
AccountId address = 1; // app's address
LayerNumber layer_number = 2; // requested value as of a layer. Pass MAX_UI32 to get latest value
uint32 id = 3; // app's var id - var's unique identifier
Copy link
Member

Choose a reason for hiding this comment

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

Where does the caller look this up?

Copy link
Member Author

Choose a reason for hiding this comment

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

Caller has the app's schema which includes the variable ids.

@avive
Copy link
Member Author

avive commented Jun 29, 2021

@lrettig @YaronWittenstein - I've updated the api to reflect the design review session feedback. I still believe we should have a strongly typed GetVariable() method which is very useful for quick access to app state by dapps. Perhaps we can proceed before another review session by you guys just reviewing the most recent changes? The relent types are in app_types.proto, var_types.proto and global_state.proto

proto/spacemesh/v1/api_config.yaml Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/app_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/global_state_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/global_state_types.proto Outdated Show resolved Hide resolved
proto/spacemesh/v1/types.proto Show resolved Hide resolved
proto/spacemesh/v1/types.proto Show resolved Hide resolved
proto/spacemesh/v1/var_types.proto Outdated Show resolved Hide resolved
@avive
Copy link
Member Author

avive commented Jul 6, 2021

@YaronWittenstein @lrettig - please check the code - I think I've incorporated everything we talked about and removed non-app related changes from this pr.

string target_svm_version = 4; // at deploy time
string kind = 5; // e.g. WASM
repeated bytes code = 6; // wasm code
string target_svm_version = 1; // at deploy time
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 this should be an int not a string

string kind = 5; // e.g. WASM
repeated bytes code = 6; // wasm code
string target_svm_version = 1; // at deploy time
string kind = 2; // e.g. WASM
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 be an enum not a string

Code code = 2; // template wasm code. Provided if caller specified to get code.
bytes metadata = 3; // storage layout, api, header. Provided if caller specified to get metadata and template include metadata.
Code code = 1; // template wasm code. Provided if caller specified to get code.
bytes metadata = 2; // storage layout, api, header. Provided if caller specified to get metadata and template include metadata.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bytes metadata = 2; // storage layout, api, header. Provided if caller specified to get metadata and template include metadata.
bytes metadata = 2; // storage layout, api, header. Provided if caller specified to get metadata and template including metadata. Caller is responsible for decoding.

}

enum TemplateDataFlags {
TEMPLATE_DATA_FLAGS_UNSPECIFIED = 0;
TEMPLATE_DATA_FLAGS_CODE = 1;
TEMPLATE_DATA_FLAGS_METADATA = 2;
// may need deploy data section - tbd - based on deploy tx format. Understand eth one.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// may need deploy data section - tbd - based on deploy tx format. Understand eth one.
// TODO: may need deploy data section - tbd - based on deploy tx format. Understand eth one.

AccountId address = 1; // App's address in global space
string name = 2; // short descriptive name (optional)
TemplateAddress template_address = 3; // App's template address
LayerNumber spawn_layer = 4; // Layer in which app was spawned
Copy link
Member

Choose a reason for hiding this comment

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

as discussed today, these also belong in metadata, as they're not required by SVM. they can optionally be automatically populated at deploy time if the deployer requests this, and pays for it.

repeated Template templates = 2;
}

// A request to get an app's variable by var id
Copy link
Member

Choose a reason for hiding this comment

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

@YaronWittenstein said these are called "storage variables" in SVM

TRANSACTION_RESULT_INSTANTIATION_FAILED = 9; // error creating an instance from a template
TRANSACTION_RESULT_DEPLOY_FAILED = 10; // error deploying a template to the mesh
TRANSACTION_RESULT_RUNTIME_EXCEPTION = 2; // app code execution exception=
TRANSACTION_RESULT_VALIDATION_FAILED = 3; // TBD
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TRANSACTION_RESULT_VALIDATION_FAILED = 3; // TBD
TRANSACTION_RESULT_VALIDATION_FAILED = 3; // TODO: do we need this? can an invalid tx ever make it into a superblock?

@avive avive changed the title Sm0.3 update (WIP) API 2.0 Jul 6, 2021
@avive
Copy link
Member Author

avive commented Jul 6, 2021

Renamed pr to api 2.0 as it is not going to be backward compatible with 1.x api due to some breaking changes.

@lrettig
Copy link
Member

lrettig commented Jul 6, 2021

Renamed pr to api 2.0 as it is not going to be backward compatible with 1.x api due to some breaking changes.

I think we need to update this so the output proto files go into proto/spacemesh/v2 (rather than v1), that should cause the CI tests to pass. We also need to add a separate set of checks for v2, and we should also update the README accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants