Skip to content
This repository was archived by the owner on Jul 5, 2024. It is now read-only.

Fix circuits integration tests panic: "unwrap on None value" when EIP-4896 withdrawals==None#1688

Merged
AronisAt79 merged 1 commit intomainfrom
Fix-Integration-Tests-Error
Nov 27, 2023
Merged

Fix circuits integration tests panic: "unwrap on None value" when EIP-4896 withdrawals==None#1688
AronisAt79 merged 1 commit intomainfrom
Fix-Integration-Tests-Error

Conversation

@AronisAt79
Copy link
Copy Markdown
Contributor

Description

Integration Tests have been failing since introduction of #1369, due to Option::Unwrap on None value, when eth_block.withdrawals==None.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Contents

  • zkevm-circuits/zkevm-circuits/src/witness/block.rs

Rationale

replace eth_block.withdrawals.clone().unwrap() with eth_block.withdrawals.clone().unwrap_or_default()

How Has This Been Tested?

https://github.com/privacy-scaling-explorations/zkevm-circuits/actions/runs/6968928576/job/18963801769


Issue

#1687

@github-actions github-actions Bot added the crate-zkevm-circuits Issues related to the zkevm-circuits workspace member label Nov 23, 2023
Copy link
Copy Markdown
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

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

LGTM. I see the unwrap panic in the CI run referenced in the issue is fixed in the run referenced in your pull request description

@AronisAt79
Copy link
Copy Markdown
Contributor Author

Thanks @ChihChengLiang . Yes the unwrap_or_default clears the error so if this solution is acceptable, we can merge this fix. I would like though to have another review from @KimiWu123 cause integration tests still fail down the process. evm and super circuit tests fail for erc20_openzeppelin_transfers with panic

circuit fixed columns are not constant for different witnesses

just to make sure this failure is not somehow logically associated to the fix provided with this PR. If so, we can take care of the new panic with a new issue :)

Copy link
Copy Markdown
Contributor

@KimiWu123 KimiWu123 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your fix!

@AronisAt79 AronisAt79 added this pull request to the merge queue Nov 27, 2023
Merged via the queue into main with commit e39d55c Nov 27, 2023
@AronisAt79 AronisAt79 deleted the Fix-Integration-Tests-Error branch November 27, 2023 13:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

crate-zkevm-circuits Issues related to the zkevm-circuits workspace member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants