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:remediations chainlight 001, 002, 004 #20

Closed
wants to merge 9 commits into from

Conversation

livingrockrises
Copy link
Contributor

No description provided.

@@ -47,7 +47,8 @@ contract BiconomySponsorshipPaymaster is
// Offset in PaymasterAndData to get to PAYMASTER_ID_OFFSET
uint256 private constant PAYMASTER_ID_OFFSET = PAYMASTER_DATA_OFFSET;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, any specific reason to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just for readability
PAYMASTER_DATA_OFFSET is always fixed.
PAYMASTER_ID_OFFSET depends on design how our paymasterandData is structured.

@livingrockrises
Copy link
Contributor Author

can we also check CI build issue in this PR itself.
I also don't think we can tackle coverage at this point because this repo only has foundry and we must use viIR.

@filmakarov
Copy link
Collaborator

can we also check CI build issue in this PR itself.

The build issue for me was resolved with downgrading yarn to 1.2.xx

I also don't think we can tackle coverage at this point because this repo only has foundry and we must use viIR.

why do we must use via-IR?

1 similar comment
@filmakarov
Copy link
Collaborator

can we also check CI build issue in this PR itself.

The build issue for me was resolved with downgrading yarn to 1.2.xx

I also don't think we can tackle coverage at this point because this repo only has foundry and we must use viIR.

why do we must use via-IR?

@livingrockrises
Copy link
Contributor Author

isValidSignatureNow

check getHash. stack too deep.

@filmakarov
Copy link
Collaborator

check getHash. stack too deep.

getHash can be fixed.

userOp.nonce,
                keccak256(userOp.initCode),
                keccak256(userOp.callData),
                userOp.accountGasLimits,
             uint256(bytes32(userOp.paymasterAndData[PAYMASTER_VALIDATION_GAS_OFFSET:PAYMASTER_DATA_OFFSET])),
                userOp.preVerificationGas,
                userOp.gasFees,

this can be packed in one bytes

however, there are further stack too deep errors in the token paymaster

So, the best way to handle is would be this

forge coverage --ir-minimum => here

@livingrockrises
Copy link
Contributor Author

check getHash. stack too deep.

getHash can be fixed.

userOp.nonce,
                keccak256(userOp.initCode),
                keccak256(userOp.callData),
                userOp.accountGasLimits,
             uint256(bytes32(userOp.paymasterAndData[PAYMASTER_VALIDATION_GAS_OFFSET:PAYMASTER_DATA_OFFSET])),
                userOp.preVerificationGas,
                userOp.gasFees,

this can be packed in one bytes

however, there are further stack too deep errors in the token paymaster

So, the best way to handle is would be this

forge coverage --ir-minimum => here

will use --ir-minimum then.
however we can change getHash code if we want.

@livingrockrises livingrockrises changed the title fix: remediations - test update fix: remediations Oct 11, 2024
@livingrockrises
Copy link
Contributor Author

check getHash. stack too deep.

getHash can be fixed.

userOp.nonce,
                keccak256(userOp.initCode),
                keccak256(userOp.callData),
                userOp.accountGasLimits,
             uint256(bytes32(userOp.paymasterAndData[PAYMASTER_VALIDATION_GAS_OFFSET:PAYMASTER_DATA_OFFSET])),
                userOp.preVerificationGas,
                userOp.gasFees,

this can be packed in one bytes
however, there are further stack too deep errors in the token paymaster
So, the best way to handle is would be this
forge coverage --ir-minimum => here

will use --ir-minimum then. however we can change getHash code if we want.

I will fix this in another PR to revamp test cases.

@livingrockrises livingrockrises changed the title fix: remediations fix:remediations chainlight 001, 002, 004 Oct 11, 2024
@livingrockrises
Copy link
Contributor Author

closing in favour of #24

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