Refactor/refactor provable types #388
Conversation
|
I've updated the commits - there might be missing parts in codebase, but most of the changes will be done in this manner. I'd like to have your opinion on a decision: The fields of |
docs: remove mistakenyl pushed file
| }; | ||
| } | ||
|
|
||
| export function networkStateToFields(json: NetworkStateJson): Field[] { |
There was a problem hiding this comment.
This function shouldn't exist - or at least the implementation should be NetworkState.toFields(NetworkState.fromJSON())
| return [Field(json.block.height), Field(json.previous.rootHash)]; | ||
| } | ||
|
|
||
| export function hashNetworkState(json: NetworkStateJson): string { |
There was a problem hiding this comment.
Same here, we already implement hash() in the provable version, so we should use this to not run into inconsistencies
| hash: Field(blockData.hash), | ||
| height: Field(blockData.height), | ||
| hash: blockData.hash, | ||
| height: blockData.height.toString(), |
There was a problem hiding this comment.
height is number and can stay number imo - no need to convert into strings here
| } | ||
|
|
||
| interface PendingTransactionJSONType { | ||
| export interface PendingTransactionJSONType { |
There was a problem hiding this comment.
So here, since PendingTransaction is never used inside circuits, we can just get rid of this type, and instead make PendingTransaction non-provable. That can get tricky with the signatures - we can keep UnsignedTransaction provable for now? Or create a new transaction creation API altogether
| AppChainModule, | ||
| } from "@proto-kit/sequencer"; | ||
| import { NetworkState } from "@proto-kit/protocol"; | ||
| import { NetworkStateJson } from "@proto-kit/protocol"; |
There was a problem hiding this comment.
I'm a bit unsure, is the naming (NetworkState, NetworkStateJson) or (ProvableNetworkState, NetworkState) better?
| ), | ||
| startingStateBeforeHook: input.params.startingStateBeforeHook, | ||
|
|
||
| startingStateAfterHook: input.params.startingStateAfterHook, |
|
|
||
| /** | ||
| * Serializer for RuntimeProofParametersJson. | ||
| * Since RuntimeProofParametersJson is already JSON-compatible, this is trivial. |
There was a problem hiding this comment.
Indeed this is trivial, hence why we have a JsonTaskSerializer that does this already - let's remove this one and replace usage with the JsonTaskSerializer
|
|
||
| export type TaskStateRecord = Record<string, Field[]>; | ||
|
|
||
| export type TaskStateRecordJson = Record<string, string[]>; |
There was a problem hiding this comment.
again - replace the existing struct
|
|
||
| export type TaskStateRecordJson = Record<string, string[]>; | ||
|
|
||
| export function taskStateRecordToJson( |
There was a problem hiding this comment.
This should never live in a component like this - either implement a TaskSerializer or change the methods that create TaskStateRecords to do the serialization
| tx.stateTransitions[1].stateTransitions | ||
| ); | ||
| ): RuntimeProofParametersJson { | ||
| const stBatch = STBatchFromJson(tx.stateTransitions[1]); |
There was a problem hiding this comment.
Just seeing this - again, no standalone mapping functions, and then the naming case...
Changes
In this PR, it is aimed to convert from provable types to non-provable types in out-of-circuit code.
Starting point changes in this refactoring was to implement or convert to JSON type of the followings:
BlockBlockWithResultAnd since they are
TransactionExecutionResultPendingTransactionStateTransitionBatchUntypedStateTransitionNetworkStateEspecially last 3 is used all around the Sequencing and Tracing modules.
Commits
UntypedStateTransition & StateTransitionBatch
UntypedStateTransitionJson0199877 ( rename: 5934a13 )TransactionExecutionResult
TransactionExecutionResultJsonb017bb5PendingTransaction
PrivateMempoold3c9336NetworkState
NetworkStateJsonby inferring it from Struct c29a51dRuntimeProofParametersJsontype withNetworkStateJsonandPendingTransactionJsonTypea4ac392Miscellanous
#### Test
This PR closes #201