Skip to content
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

Fix Fetch.fetchActions and add new fetchActions method for external use #843

Merged
merged 3 commits into from
Apr 11, 2023

Conversation

Comdex
Copy link
Contributor

@Comdex Comdex commented Apr 11, 2023

  • Fix Fetch.fetchActions

When sending actions in the following ways, an error will be reported when using the existing getActions method to get actions data:

let tx = await Mina.transaction(
        {
          sender: feePayerAddress,
          fee: txFee,
        },
        () => {
          zkapp.doSomething(); // dispatch 1 action
          zkapp.doSomething(); // dispatch 1 action
        }
);

999

This problem only occurs when there are multiple actions corresponding to multiple AccountUpates in the same transaction, which is basically caused by a bug in Fetch.fetchActions that the getActions method depends on.

It can be reproduced using snarkyjs version 0.9.6 under the current Berkeley network:

 // This fetchActions method is not exported for external use in current version
 let actions = await fetchActions({
        publicKey: 'B62qqfgrF56mGyqNXsyd6iMqwWDQVC9KKcLVwcToLwfNg5ju2tVwSn5',
        actionStates: {
          fromActionState: Reducer.initialActionsHash.toString(),
        },
      });
 console.log('actions: ', JSON.stringify(actions));
  • Add two new fetchActions methods for exporting to be used externally

Currently, we don't have any methods that can be called outside the smart contract to get actions from the network.
So it is proposed to add two new asynchronous methods to solve this problem(Compatible with LocalBlockChain and Network):

// Mainly returns data in string form
Mina.fetchActions(publicKey, {fromActionState, endActionState}, tokenId)
// Return data as ActionType
SmartContract.reducer.fetchActions({
    fromActionState,
    endActionState,
})

Copy link
Member

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Comdex for adding the new fetch functions! Having them is consistent with the rest of the API, and useful.

I'm a bit worried that if there was a bug in fetchActions, this PR doesn't fix it

Comment on lines 825 to +827
if (isSameAccountUpdate && !isLastAction) {
currentActionList.push(data);
currentAccountUpdateId = accountUpdateId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to fix the bug that you mention? If so, I don't see how it can. The if branch is only executed when we had accountUpdateId === currentAccountUpdateId, so the new line currentAccountUpdateId = accountUpdateId; does nothing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a simple fix, in fact you can see that the currentAccountUpdateId will be updated after the other branches end, and this problem is caused by the lack of this processing due to the direct return

Copy link
Member

@mitschabaude mitschabaude Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. Can you explain how setting currentAccountUpdateId to accountUpdateId can change anything if currentAccountUpdateId is already equal to accountUpdateId, which was tested in the line above (isSameAccountUpdate)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitschabaude Got you, I made a mistake, lol 😂
Please ignore this modification
I will make a new pull request with the correct fix

@mitschabaude mitschabaude merged commit 9c2b94d into o1-labs:main Apr 11, 2023
@Comdex Comdex mentioned this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants