-
Notifications
You must be signed in to change notification settings - Fork 4
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
adjust query to support ordering #107
Changes from 5 commits
5efccc8
f01a38e
030c60f
01f0a70
5bc4124
6c1a253
e1fc23d
3c0750e
73b7158
71bc454
7f51ad5
03c0cb1
f8336ef
7020459
71e84e7
a6eb6f9
29c936b
49cb8b0
d3a6b51
5e706f7
43feb71
231a9ca
f959060
fb9be4b
ac04af1
a007e8a
0aeec84
335dffa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,7 +124,10 @@ export type TransactionInfo = { | |
authorizationKind: Scalars['String']['output']; | ||
hash: Scalars['String']['output']; | ||
memo: Scalars['String']['output']; | ||
sequenceNumber: Scalars['Int']['output']; | ||
status: Scalars['String']['output']; | ||
zkappAccountUpdateIds: Array<Maybe<Scalars['Int']['output']>>; | ||
zkappEventElementIds: Array<Maybe<Scalars['Int']['output']>>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ended up just removing these. I don't think we need to expose them in graphQL after all |
||
}; | ||
|
||
export type ResolverTypeWrapper<T> = Promise<T> | T; | ||
|
@@ -448,7 +451,18 @@ export type TransactionInfoResolvers< | |
>; | ||
hash?: Resolver<ResolversTypes['String'], ParentType, ContextType>; | ||
memo?: Resolver<ResolversTypes['String'], ParentType, ContextType>; | ||
sequenceNumber?: Resolver<ResolversTypes['Int'], ParentType, ContextType>; | ||
status?: Resolver<ResolversTypes['String'], ParentType, ContextType>; | ||
zkappAccountUpdateIds?: Resolver< | ||
Array<Maybe<ResolversTypes['Int']>>, | ||
ParentType, | ||
ContextType | ||
>; | ||
zkappEventElementIds?: Resolver< | ||
Array<Maybe<ResolversTypes['Int']>>, | ||
ParentType, | ||
ContextType | ||
>; | ||
__isTypeOf?: IsTypeOfResolverFn<ParentType, ContextType>; | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -143,7 +143,7 @@ class ActionsService implements IActionsService { | |
} | ||
actions.push({ | ||
blockInfo, | ||
actionData: actionsData.flat(), | ||
actionData: this.sortActions(actionsData.flat()), | ||
actionState: { | ||
/* eslint-disable */ | ||
actionStateOne: action_state_value1!, | ||
|
@@ -157,4 +157,26 @@ class ActionsService implements IActionsService { | |
} | ||
return actions; | ||
} | ||
|
||
sortActions(actions: Action[]): Action[] { | ||
return actions.sort((a, b) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since actions are returned grouped by block, this is focused on ordering the actions within a block. I'm not sure the sort order is correct. Maybe it's reversed, but we should get it right, then we will have access to the unit test as a spec. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @deepthiskumar it's worth noting that you and gregor both mentioned a sub-order to the account update in o1-labs/o1js#1872 It is your point 4, and Gregor said "It's extremely important that when reordering the actions here, the order within the same account update is maintained". This sort only looks at sequence number and account update id. But with my JSON comment on this PR and this code, we should be able to get on the same page more easily. If there's a third sorting lever we need here, then I can add it, but it's not clear to me what it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the third sorting lever is the element ID index from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So as linked in my other comment, I think we only need to sort to the |
||
// Sort by sequence number | ||
if ( | ||
a.transactionInfo.sequenceNumber !== b.transactionInfo.sequenceNumber | ||
) { | ||
return ( | ||
a.transactionInfo.sequenceNumber - b.transactionInfo.sequenceNumber | ||
); | ||
} | ||
|
||
// Sort by account update index if sequence number is the same | ||
const aIndex = a.transactionInfo.zkappAccountUpdateIds.indexOf( | ||
Number(a.accountUpdateId) | ||
); | ||
const bIndex = b.transactionInfo.zkappAccountUpdateIds.indexOf( | ||
Number(b.accountUpdateId) | ||
); | ||
return aIndex - bIndex; | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this include action ids and field element ids also? The protocol supports multiple actions per account update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, are we not doing similar sorting for events? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Within one account update, the actions are ordered within the const structData = eventData.data;
assert.strictEqual(structData.length, 16);
assert.strictEqual(structData[0], '2');
assert.deepStrictEqual(
structData.slice(1, 6),
structToAction(expectedS1)
);
assert.deepStrictEqual(
structData.slice(6, 11),
structToAction(expectedS2)
);
assert.deepStrictEqual(
structData.slice(11, 16),
structToAction(expectedS3)
); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't changed the intra-AU event order here, as it seems to be correct already. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've always been told as a zkapp developer that event order is not guaranteed, so I don't think we need to. The only reason we need to sort actions is because an actions hash is committed to, and that hash uses a particular order that needs to be reproduced. There is no such events hash on chain, so no ordering should be necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The query here to fetch actions uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I'll double check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @deepthiskumar we convert field arrays into Maybe I can add a test to make sure this order is explicitly respected, but I think it happens to work correctly already. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add such a test? I think that'll help a lot with order being preserved implicitly. Everything else looks good otherwise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I will add a test in a fast-follow! |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import { createYoga, createSchema } from 'graphql-yoga'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole file may be better to remove. It's just a script to make a graphql request against the local API. |
||
import { loadSchemaSync } from '@graphql-tools/load'; | ||
import { GraphQLFileLoader } from '@graphql-tools/graphql-file-loader'; | ||
import { buildHTTPExecutor } from '@graphql-tools/executor-http'; | ||
import { parse } from 'graphql'; | ||
import { resolvers } from '../src/resolvers.js'; | ||
import { buildContext, GraphQLContext } from '../src/context.js'; | ||
|
||
const PG_CONN = 'postgresql://postgres:postgres@localhost:5432/archive '; | ||
const zkappAccount = 'B62qmBrPiukbHj4VnXdgMzjj2zpoQZeSBxZA6JDYMeeShApRAKaorto'; | ||
|
||
const actionsQuery = ` | ||
query getActions($input: ActionFilterOptionsInput!) { | ||
actions(input: $input) { | ||
blockInfo { | ||
stateHash | ||
timestamp | ||
height | ||
parentHash | ||
chainStatus | ||
distanceFromMaxBlockHeight | ||
globalSlotSinceGenesis | ||
} | ||
actionState { | ||
actionStateOne | ||
actionStateTwo | ||
actionStateThree | ||
actionStateFour | ||
actionStateFive | ||
} | ||
actionData { | ||
data | ||
accountUpdateId | ||
transactionInfo { | ||
status | ||
hash | ||
memo | ||
sequenceNumber | ||
zkappAccountUpdateIds | ||
zkappEventElementIds | ||
} | ||
} | ||
} | ||
} | ||
`; | ||
|
||
const schema = createSchema({ | ||
typeDefs: loadSchemaSync('./schema.graphql', { | ||
loaders: [new GraphQLFileLoader()], | ||
}), | ||
resolvers, | ||
}); | ||
const context = await buildContext(PG_CONN); | ||
const yoga = createYoga<GraphQLContext>({ schema, context }); | ||
const executor = buildHTTPExecutor({ | ||
fetch: yoga.fetch, | ||
}); | ||
|
||
const results = await executor({ | ||
variables: { | ||
input: { address: zkappAccount }, | ||
}, | ||
document: parse(`${actionsQuery}`), | ||
}); | ||
|
||
console.log(JSON.stringify(results)); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ import { loadSchemaSync } from '@graphql-tools/load'; | |
import { GraphQLFileLoader } from '@graphql-tools/graphql-file-loader'; | ||
import { buildHTTPExecutor } from '@graphql-tools/executor-http'; | ||
import { parse } from 'graphql'; | ||
import { PrivateKey, Lightnet } from 'o1js'; | ||
import { PrivateKey, Lightnet, Mina } from 'o1js'; | ||
import { resolvers } from '../src/resolvers.js'; | ||
import { buildContext, GraphQLContext } from '../src/context.js'; | ||
import { | ||
|
@@ -15,8 +15,10 @@ import { | |
emitSingleEvent, | ||
setNetworkConfig, | ||
Keypair, | ||
emitActionsFromMultipleSenders, | ||
} from '../zkapp/utils.js'; | ||
import { HelloWorld } from '../zkapp/contract.js'; | ||
import { Actions } from 'src/blockchain/types.js'; | ||
|
||
const eventsQuery = ` | ||
query getEvents($input: EventFilterOptionsInput!) { | ||
|
@@ -69,6 +71,8 @@ query getActions($input: ActionFilterOptionsInput!) { | |
status | ||
hash | ||
memo | ||
sequenceNumber | ||
zkappAccountUpdateIds | ||
} | ||
} | ||
} | ||
|
@@ -85,27 +89,31 @@ describe('Query Resolvers', async () => { | |
let zkApp: HelloWorld; | ||
|
||
before(async () => { | ||
setNetworkConfig(); | ||
try { | ||
setNetworkConfig(); | ||
|
||
const schema = createSchema({ | ||
typeDefs: loadSchemaSync('./schema.graphql', { | ||
loaders: [new GraphQLFileLoader()], | ||
}), | ||
resolvers, | ||
}); | ||
const context = await buildContext(PG_CONN); | ||
const yoga = createYoga<GraphQLContext>({ schema, context }); | ||
executor = buildHTTPExecutor({ | ||
fetch: yoga.fetch, | ||
}); | ||
const schema = createSchema({ | ||
typeDefs: loadSchemaSync('./schema.graphql', { | ||
loaders: [new GraphQLFileLoader()], | ||
}), | ||
resolvers, | ||
}); | ||
const context = await buildContext(PG_CONN); | ||
const yoga = createYoga<GraphQLContext>({ schema, context }); | ||
executor = buildHTTPExecutor({ | ||
fetch: yoga.fetch, | ||
}); | ||
|
||
zkAppKeypair = await Lightnet.acquireKeyPair(); | ||
senderKeypair = await Lightnet.acquireKeyPair(); | ||
zkApp = await deployContract( | ||
zkAppKeypair, | ||
senderKeypair, | ||
/* fundNewAccount = */ false | ||
); | ||
zkAppKeypair = await Lightnet.acquireKeyPair(); | ||
senderKeypair = await Lightnet.acquireKeyPair(); | ||
zkApp = await deployContract( | ||
zkAppKeypair, | ||
senderKeypair, | ||
/* fundNewAccount = */ false | ||
); | ||
} catch (error) { | ||
console.error(error); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This console was helpful in debugging. Errors were swallowed otherwise. |
||
} | ||
}); | ||
|
||
after(async () => { | ||
|
@@ -198,7 +206,7 @@ describe('Query Resolvers', async () => { | |
}); | ||
}); | ||
|
||
describe('Actions', async () => { | ||
describe.only('Actions', async () => { | ||
test('Fetching actions with a valid address should not throw', async () => { | ||
assert.doesNotThrow(async () => { | ||
await executor({ | ||
|
@@ -253,5 +261,45 @@ describe('Query Resolvers', async () => { | |
const lastAction = actions[actions.length - 1]; | ||
assert.strictEqual(lastAction.actionData.length, 3); | ||
}); | ||
|
||
describe('Actions from different accounts', async () => { | ||
const sendersCount = 5; | ||
const actionsCount = 3; | ||
const senders: Keypair[] = []; | ||
|
||
before(async () => { | ||
for (let i = 0; i < sendersCount; i++) { | ||
senders.push(await Lightnet.acquireKeyPair()); | ||
} | ||
}); | ||
|
||
test('Emitting actions from many accounts should be fetchable in o1js', async () => { | ||
await emitActionsFromMultipleSenders(zkApp, senders, { | ||
numberOfEmits: actionsCount, | ||
}); | ||
|
||
await Mina.fetchActions(zkApp.address); // This line will throw if actions do not reproduce the correct action hash | ||
assert(true); | ||
}); | ||
|
||
test('Fetched actions have order metadata', async () => { | ||
const results = await executor({ | ||
variables: { | ||
input: { | ||
address: zkApp.address, | ||
}, | ||
}, | ||
document: parse(`${actionsQuery}`), | ||
}); | ||
const actions: Actions = results.data.actions; | ||
for (const block of actions) { | ||
const actionData = block.actionData; | ||
for (const action of actionData) { | ||
assert.ok(action.transactionInfo.sequenceNumber); | ||
assert(action.transactionInfo.zkappAccountUpdateIds.length > 0); | ||
} | ||
} | ||
}); | ||
}); | ||
}); | ||
}); |
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.
You'll want another similar field for Events (this is for Actions) to include as part of the Events data, no?
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 we need that, assuming events will not be sorted?
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 what you mean by this. Events are recorded in the same order as actions i.e., sequence of blocks, sequence of transactions within a block, sequence of account updates within a transaction and sequence of events within an account update. The only difference is that the actions are accumulated on chain in action state and so one will always want to know the order of actions to validate against action states. For events it is more of a utility where an app might want to process events in the order they were confirmed/included in the chain. As an AP,I it seems like it should be providing that utility