-
Notifications
You must be signed in to change notification settings - Fork 15
Refactor/refactor provable types #388
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
base: develop
Are you sure you want to change the base?
Conversation
packages/sequencer/src/protocol/production/sequencing/BlockProducerModule.ts
Outdated
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/tracing/BlockTracingService.ts
Outdated
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/helpers/UntypedOption.ts
Outdated
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/helpers/UntypedOption.ts
Outdated
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/helpers/UntypedStateTransition.ts
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/helpers/UntypedStateTransition.ts
Outdated
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/helpers/UntypedStateTransition.ts
Outdated
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/helpers/UntypedStateTransition.ts
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/sequencing/BlockResultService.ts
Outdated
Show resolved
Hide resolved
packages/sequencer/src/protocol/production/sequencing/BlockResultService.ts
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| console.log("tx nonce", tx.transaction?.nonce.toBigInt()); | ||
| console.log("tx nonce", (tx.transaction as PendingTransaction).nonce); |
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.
Why do you do this? tx should already be of type PendingTransaction, no?
| import { ObjectMapper } from "../../../ObjectMapper"; | ||
|
|
||
| type EventData = { | ||
| type EventDataJson = { |
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 should leave this as EventData - there's not provable version of it, so that name is fine imo
|
|
||
| let txHash: string; | ||
|
|
||
| if (transaction.transaction instanceof PendingTransaction) { |
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.
Whaat - why do we have this conditional in a test?
| } | ||
|
|
||
| if (lastProcessedBlockHeight === Number(block?.block.height.toBigInt())) { | ||
| if (lastProcessedBlockHeight === Number(block?.block.height)) { |
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.
Can't we skip this cast to number since it's already a number?
| const args = await methodEncoder.decode(fieldArgs, []); | ||
|
|
||
| const { fields, auxiliary } = methodEncoder.encode(args); | ||
| const hash = Poseidon.hash([ |
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.
Like I mentioned in PendingTransaction.ts, we shouldn't do this!
| latestBlockWithResult.result.afterNetworkState.hash().toString() | ||
| ).toStrictEqual(batch!.toNetworkState.hash().toString()); | ||
| new ProvableNetworkState( | ||
| ProvableNetworkState.fromJSON( |
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 have these 5 lines at least 20 times in the codebase - should we make NetworkState a class and implement toProvable and fromProvable on it?
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.
Not sure - even though toProvable and fromProvable will be good to remove these parts, we could need fromJson and toJson in that class too.
Maybe not though, have to check where NetworkState is used.
| interface PendingTransactionJSONType { | ||
| hash: string; | ||
| methodId: string; | ||
| nonce: string; |
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.
Why is nonce a string? It should be a number
| async function createBatch( | ||
| withTransactions: boolean, | ||
| customNonce: number = 0, | ||
| // Why is it like this? |
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.
What are you referring to?
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.
A note for myself that where i thought of using PendingTransaction as json, this line will be removed.
|
|
||
| const txHash = tx.transaction?.hash().toString()!; | ||
| const txHash = | ||
| tx.transaction instanceof UnsignedTransaction |
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.
Yeah no, we need to fix the PendingTransaction implementation :)
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