[ON HOLD] Feature: Full declare_native_blueprint_state macro
#1318
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dhedey
force-pushed
the
feature/key-data-model-versioning
branch
3 times, most recently
from
6a7c239
to
69e29e2
Compare
Generics are now supported, as is a simplified define_single_versioned
dhedey
force-pushed
the
feature/key-data-model-versioning
branch
from
69e29e2
to
24aaef8
Compare
dhedey
changed the base branch from
develop
to
data-model-versioning-branch-point
Benchmark for 0f3006cClick to view benchmark
|
dhedey
changed the base branch from
data-model-versioning-branch-point
to
develop
talekhinezh
reviewed
Aug 17, 2023
| use sbor::LocalTypeIndex; | ||
| use crate::internal_prelude::*; | ||
|
|
||
| define_wrapped_hash!( |
| @@ -195,7 +188,7 @@ pub fn to_typed_substate_key( | |||
| } | |||
| SCHEMAS_PARTITION => { | |||
| let key = substate_key.for_map().ok_or_else(|| error("Schema key"))?; | |||
| TypedSubstateKey::Schema(TypedSchemaSubstateKey::SchemaKey( | |||
| TypedSubstateKey::SchemaModule(TypedSchemaSubstateKey::SchemaKey( | |||
There was a problem hiding this comment.
Minor: I wouldn't consider this a module. In our model a module is specifically something with logic/methods. In this case the schemas are just there as data for use.
| } | ||
|
|
||
| /// This is used mostly for flashing | ||
| pub fn into_kernel_main_partitions(self, feature_checks: [<$blueprint_ident FeatureChecks>]) -> Result<NodeSubstates, RuntimeError> { |
There was a problem hiding this comment.
I don't think this is right place for doing kernel mappings. The problem is that this mapping now needs to be maintained in multiple places and enforces the requirement that the mapping is static and may never change in the future, losing flexibility/updatability. The proper mechanism instead would be to use "System Layer" code to do this mapping.
| ); | ||
| } | ||
| let mut partitions = package_state_init | ||
| .into_kernel_main_partitions(own_features.into()) |
There was a problem hiding this comment.
It shouldn't be the PackageStateInit which should decide where things are mapped, an example of why this model is off.
| @@ -738,139 +589,26 @@ pub fn create_bootstrap_package_partitions( | |||
|
|
|||
| fn globalize_package<Y>( | |||
| package_address_reservation: Option<GlobalAddressReservation>, | |||
| package_structure: PackageStructure, | |||
| mut package_state_init: PackageStateInit, | |||
| blueprint_version_auth_configs, | ||
| code_vm_type: vm_type_substates, | ||
| code_original_code: original_code_substates, | ||
| code_instrumented_code: instrumented_code_substates, |
| @@ -1493,16 +1154,16 @@ impl PackageRoyaltyNativeBlueprint { | |||
| if royalty_charge.is_non_zero() { | |||
| let handle = api.kernel_open_substate( | |||
| receiver, | |||
| MAIN_BASE_PARTITION, | |||
| &PackageField::Royalty.into(), | |||
| PackagePartition::Field.as_main_partition(), | |||
There was a problem hiding this comment.
Again, would rather not rely on outside system code to specify what the main_partition is.
| type Error = (); | ||
|
|
||
| fn try_from(offset: PartitionOffset) -> Result<Self, Self::Error> { | ||
| // NOTE: This should really be a fixed mapping - but it's hard to do with declarative macros |
There was a problem hiding this comment.
The problem is this mapping really shouldn't be done here.
| description: "Enables the package royalty substate", | ||
| } | ||
| }, | ||
| instance_schema_types: {}, |
dhedey
added
the
substate
dhedey
changed the title
dhedey
changed the title
declare_native_blueprint_state macro
dhedey
changed the title
declare_native_blueprint_state macrodeclare_native_blueprint_state macro
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR status update
This full PR has been parked, and instead, will do it in bits (starting with just the core versioned models + schema stuff) - will create a new PR to capture that.
Replacement PR 2023: #1358
Extension PR 2024: #1744
Summary
The goal of this PR is to introduce a more structured / type-safe mechanism for interfacing with our native substates... and to enable easily wrapping them with lazily-upgradable versioned enum wrappers in a type-safe way.
This PR has:
versioned!macro to make versioning easydeclare_native_blueprint_statemacro to create typed native substate models which creates:Generic "Versioned" data model
In terms of versioning conventions, I believe we are going to:
When implementing for X = Schema, I decided the following naming schema fitted best - keeping code looking as neat as possible:
VersionedXis the versioned enum type. This will be used for substate contents, or other types which need versioning.Xis a type alias created for "latest version of this type"XV1is the concrete type which should be created, which maps to the type alias.Questions to be answered before proceeding:
Changes for integrators
I'll merge this into the node. This shouldn't have any impact outside of Scrypto and Node.