-
Notifications
You must be signed in to change notification settings - Fork 12
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
Implement unproven blocks #72
Conversation
@@ -130,7 +130,8 @@ | |||
"@typescript-eslint/no-unnecessary-condition": [ | |||
"off" | |||
], | |||
"nodejs/declare": "off" | |||
"nodejs/declare": "off", | |||
"unicorn/prefer-event-target": "off" |
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.
out of curiousity, where was this rule triggered? i couldn't find 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.
I named the EventEmitter interface the same way, but this plugin was so badly engineered so that it confused those two
… into feature/unproven-blocks
return new ComputedBlockModel( | ||
txs.map((tx) => ComputedBlockTransactionModel.fromServiceLayerModel(tx)), | ||
JSON.stringify(proof.toJSON()) | ||
proof.proof === "mock-proof" |
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.
should import mockProof
from provableMethod.ts
import { ComputedBlockTransactionModel } from "./BlockStorageResolver"; | ||
|
||
@ObjectType() | ||
export class UnprovenBlockModel { |
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.
are we sticking with UnprovenBlock or ComputedBlock?
I think the naming for unproven/proven could be one of these for example:
- ComputedBlock / ProvenBlock
- Block / ProvenBlock
I think right now we have ComputedBlock, UnprovenBlock, Block?
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.
Agreed to in discord, will be a follow-up PR
|
||
public emit(event: keyof Events, ...parameters: Events[typeof event]) { | ||
const listeners = this.listeners[event]; | ||
if(listeners !== undefined) { |
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.
space missing :/
[key in keyof Events]?: ((...args: Events[key]) => void)[]; | ||
}; | ||
|
||
export class EventEmitter<Events extends EventsRecord> { |
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 really don't think we should be writing our own event emitter, why not use something like this: https://www.npmjs.com/package/event-emitter
Even if we don't, we should add .off(...)
to the event emitter.
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.
No Generics in this packages :/
Added .off()
[key: string]: EventSignature; | ||
} | ||
|
||
export interface EventEmittingComponent<Events extends EventsRecord> { |
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.
could have been EventEmittable
@@ -0,0 +1,11 @@ | |||
import type { EventEmitter } from "./EventEmitter"; | |||
|
|||
export type EventSignature = unknown[]; |
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 is an event signature? just the payload of the event itself?
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.
Exactly, renamed it
* Toggles whether on tracing, the block and state transitions provers | ||
* should run a simulation | ||
*/ | ||
simulateProvers?: boolean; |
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 haven't seen this consumed anywhere - what exactly should this do? disable proofs in workers? isn't this just another instance of AppChain's proofsEnabled?
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 file probably can be removed
|
||
// Clear executionContext | ||
executionContext.afterMethod(); | ||
executionContext.clear(); |
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.
doesnt the @runtimeMethod decorator handle all lifecycle details for the context?
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.
It should, I tried removing it, and I get merkle witness errors, so I'd leave it in for now
const balance = this.balances.get(address); | ||
|
||
log.provable.debug("Balance:", balance.isSome, balance.value); | ||
|
||
const newBalance = balance.value.add(value); | ||
const newBalance = balance.orElse(UInt64.zero).add(value); |
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.
there's an implicit default value thats zero, why did you have to change the test here?
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.
good q, can't remember :(
No description provided.