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

Handle state sync execution failures properly #1676

Conversation

Stefan-Ethernal
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal commented Jun 28, 2023

Description

Failed state sync events execution is handled properly now. Instead of expecting only failed transaction receipt, we should attest whether there is StateSyncResult with the Success field set to false. Those state sync events are also considered as failed state sync events.

The reason behind it is that StateReceiver invokes a target contract (in this case NativeERC20), without having a require, but instead only marking it as failed and emitting appropriate StateSyncResult:
https://github.com/0xPolygon/core-contracts/blob/main/contracts/child/StateReceiver.sol#L165-L170

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

  1. remove premining 0x0 address from cluster script and run it
  2. when the cluster it is up and running, send deposit transactions using the following command:
go run . bridge deposit-erc20 --sender-key aa75e9a7d427efc732f8e4f1a5b7646adcc61fd5bae40f80d13c8419c9f43d6d \
	--amounts 1000000000000000000 \
	--receivers <receiver address on Supernets> \
	--root-token <native erc20 address retrieved from genesis.json> \
	--root-predicate <erc20 predicate address retrieved from genesis.json> \
	--json-rpc http://localhost:8545 \
	--minter-key aa75e9a7d427efc732f8e4f1a5b7646adcc61fd5bae40f80d13c8419c9f43d6d
  1. notice that there is the following error displayed in the state sync relayer log:
    "State sync execution failed. err=failed to execute state sync id..."

@Stefan-Ethernal Stefan-Ethernal self-assigned this Jun 28, 2023
@Stefan-Ethernal Stefan-Ethernal added the bug fix Functionality that fixes a bug label Jun 28, 2023
@Stefan-Ethernal Stefan-Ethernal requested a review from a team June 28, 2023 16:41
@Stefan-Ethernal Stefan-Ethernal requested a review from a team June 28, 2023 16:51
@Stefan-Ethernal Stefan-Ethernal mentioned this pull request Jun 29, 2023
4 tasks
Copy link
Contributor

@vcastellm vcastellm left a comment

Choose a reason for hiding this comment

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

LGTM

@goran-ethernal goran-ethernal merged commit beeb461 into develop Jun 30, 2023
7 checks passed
@goran-ethernal goran-ethernal deleted the EVM-721-handle-failed-state-sync-execution-appropriately branch June 30, 2023 09:00
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug fix Functionality that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants