refactor(core): remove unnecessary accumulatorQueue in offchain instance
#866
Conversation
baumstern
force-pushed
the
newstr
branch
7 times, most recently
from
574df96 to
2138177
Compare
baumstern
changed the title
baumstern
changed the title
accumulatorQueue in offchain instance
| MaciState, | ||
| packProcessMessageSmallVals, | ||
| unpackProcessMessageSmallVals, | ||
| } from "../"; |
There was a problem hiding this comment.
I've seen you using a mix of ways of importing, for instance within Poll and MaciState you import from the actual file (i.e. import {x} from ./utils/utils) and here you import directly everything from index.ts -> Please make sure it's all consistent (not sure if anyone has a preference here on whether to import from index or from the actual file from within the same package so I'll leave that up to you to decide/find out)
There was a problem hiding this comment.
Hmm, I've been importing directly from the actual path when it's within the same package, but for external packages, I thought referencing from index.ts was the standard approach. I've also treated test files as external, even though they're in the same package directory. However, I'm not entirely certain about the most idiomatic way to handle imports in JS🤔
There was a problem hiding this comment.
@0xmad what's your take here on what's the most appropriate way to deal with imports?
There was a problem hiding this comment.
Here we need to be careful because index import usually produce cycle dependencies. From my experience, we had test forlder for each module/component and just imported from index file.
baumstern
force-pushed
the
newstr
branch
3 times, most recently
from
172ce79 to
464487f
Compare
|
@ctrlc03 Thanks for the review! I've made the revisions you suggested. Please take another look |
0xmad
changed the title
accumulatorQueue in offchain instanceaccumulatorQueue in offchain instance
0xmad
changed the title
accumulatorQueue in offchain instanceaccumulatorQueue in offchain instance
core/ts/Poll.ts
Outdated
| @@ -31,7 +30,6 @@ const blankStateLeafHash = blankStateLeaf.hash(); | |||
| interface IPoll { | |||
| topupMessage(_message: Message): void; | |||
| publishMessage(_message: Message, _encPubKey: PubKey): void; | |||
| mergeAllMessages(): void; | |||
| hasUnprocessedMessages(): boolean; | |||
| processMessages(_pollId: number): unknown; | |||
There was a problem hiding this comment.
hadn't noticed this before, but it should not be unknown, it can cause issues on other packages that depend on it
There was a problem hiding this comment.
also it looks that some functions are set to return void like tallyVotes - for now maybe just add any before we create better typings?
example of what went wrong on another PR due to this:
ts/__tests__/TallyVotes.test.ts(119,63): error TS2339: Property 'length' does not exist on type 'unknown'.
That is using the return value of tallyVotes (the circuit inputs)
✅ Deploy Preview for maci-typedoc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@ctrlc03 Please take another look! |
This PR removes the accumulatorQueue from our off-chain
MaciStateandPollinstances. Given the absence of on-chain gas constraints off-chain, this data structure is redundant. The primary mechanism for state management remains the quinary Merkle tree.