Sort actions by tx sequence when available #1917
Conversation
| if (error) throw Error(error.statusText); | ||
| if (error) { | ||
| // retry once without querying transaction info | ||
| [response, error] = await makeGraphqlRequest<ActionQueryResponse>( |
There was a problem hiding this comment.
Rather than retry the query, perhaps we can set the archive node API version in the Mina.Network settings somewhere. Then we could make a different query based on which version we believe we're talking to.
For this specific example, after we get Minascan to update their version, the new query will work in nearly every case, but we ought to prepare for the case that different versions of the API could be running.
|
This PR is not to be merged until after Minascan deploys the fixes in o1-labs/Archive-Node-API#107 |
| @@ -20,6 +20,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm | |||
| ### Fixed | |||
|
|
|||
| - Compiling stuck in the browser for recursive zkprograms https://github.com/o1-labs/o1js/pull/1906 | |||
| - Sort order for actions now includes the transaction sequence number and the exact account id sequence | |||
| '1', | ||
| ], | ||
| ], | ||
| hash: '10964420428484427410756859799314206378989718180435238943573393516522086219419', |
There was a problem hiding this comment.
given our history with the action sequence, I think we should seriously consider some advanced testing here - similar to the property testing we do in other places of the codebase
There was a problem hiding this comment.
Can you describe what cases you'd like to see? The existing tests here are simply real payloads from the graphQL node, and the real output of fetchActions in a case where the actions are derived successfully. In particular, it is one of the edge cases discovered by the original reporter of the issue: #1872 (comment)
There was a problem hiding this comment.
Take a look at this https://github.com/o1-labs/o1js/blob/00eb12df2b35cd7bc6f020570e20c5e28eee8ed5/src/lib/testing/property.ts
just something to consider
| hash: '3432500319723012006583645985859849032413742507994268064744704124629760504866', | ||
| }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
this is very long and makes it very hard to read, can we clean this up a little?
There was a problem hiding this comment.
I don't know what you mean by clean it up? Do you mean move the payload to a JSON file and read it in?
I would push back on removing this test or trying to prune it to its most useful bits. Since it is a raw input-output from a real edge case, I think it's worth leaving in full. It's not really meant to be readable, but it will flag regressions.
There was a problem hiding this comment.
I don't want to remove it but I would like to make it more readable (dedicated JSON file for the payloads, for example)
| @@ -0,0 +1,1864 @@ | |||
| export { mockFetchActionsResponse }; | |||
There was a problem hiding this comment.
see what i wrote above; this could save us a few thousand lines here too
There was a problem hiding this comment.
FWIW, this is the block in which the actions are out of order by the currently-live algorithm. So if we did trim it, we should at least keep this.
{
blockInfo: {
distanceFromMaxBlockHeight: 10037,
},
actionState: {
actionStateOne:
'11553781833537412187998791170094246504179739906426596650283148421062134031747',
actionStateTwo:
'2657110063538905863496230550574434206907731812034404344231638673775478011856',
},
actionData: [
{
accountUpdateId: '40618',
data: [
'7',
'7',
'7',
'7',
'7',
'7',
'15661593045172372274349484602975176160403678593916071611247487060653084577990',
'0',
'1',
],
transactionInfo: {
sequenceNumber: 4,
zkappAccountUpdateIds: [40618, 6396, 33066],
},
},
{
accountUpdateId: '40617',
data: [
'3',
'4',
'5',
'7',
'6',
'2',
'6453147199940115032546911372596674482366995380739961315278593325733504245402',
'0',
'1',
],
transactionInfo: {
sequenceNumber: 5,
zkappAccountUpdateIds: [40617, 40372, 40373],
},
},
{
accountUpdateId: '40619',
data: [
'3',
'4',
'5',
'7',
'6',
'3',
'21594276700301685638451290563121567472719745179788000956082286147863718718533',
'1',
'1',
],
transactionInfo: {
sequenceNumber: 6,
zkappAccountUpdateIds: [40619, 33539, 33540],
},
},
{
accountUpdateId: '40620',
data: [
'3',
'4',
'5',
'7',
'6',
'3',
'21594276700301685638451290563121567472719745179788000956082286147863718718533',
'1',
'1',
],
transactionInfo: {
sequenceNumber: 7,
zkappAccountUpdateIds: [40620, 33539, 33540],
},
},
],
},
```
There was a problem hiding this comment.
Sorry, I meant the property testing would be benefical here too #1917 (comment)
| @@ -317,7 +321,8 @@ const getActionsQuery = ( | |||
| publicKey: string, | |||
| actionStates: ActionStatesStringified, | |||
| tokenId: string, | |||
| _filterOptions?: EventActionFilterOptions | |||
| _filterOptions?: EventActionFilterOptions, | |||
| retryWithoutTxInfo: boolean = false | |||
There was a problem hiding this comment.
nit: we could generalize this into the options parameter above
Relates to #1872
The archive node currently in production does not expose enough information to accurately sort actions and produce an actions hash.
o1-labs/Archive-Node-API#107 is the PR to fix the archive node API, and this is the PR to update the query and sorting in o1js.