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 ccq #57

Merged
merged 6 commits into from
Nov 14, 2024
Merged

Fix ccq #57

merged 6 commits into from
Nov 14, 2024

Conversation

nonergodic
Copy link
Collaborator

@nonergodic nonergodic commented Sep 2, 2024

fixes:
fix bug where valid query response could be rejected during times of guardian rotation
fixed missing bounds check when parsing 4 bytes for funcSig
deleted WormholeMock (which had an empty quorum implementation => tests actually never checked correctness of quorum)

functional:
turned QueryResponse abstract contract into library
moved query types into standalone library
implemented quorum calculations directly instead of using a cross-contract call
removed superfluous guardian set not empty check
replaced string reverts for no quorum and verification failed with custom errors to unify error reporting
split validateEthCallData's implementation into two separate, independent functions
introduced usage of BytesParsing.sliceUint32PrefixedUnchecked
refactored tests to use a fork and WormholeOverride
added NoQuorum testcase

cosmetic:
reduced indent to 2 spaces
renamed QueryResponse to QueryResponseLib
renamed ParsedQueryResponse to QueryResponse
renamed ParsedPerChainQueryResponse to PerChainQueryResponse
renamed SOL_ACCOUNT to SOLANA_ACCOUNT and SOL_PDA to SOLANA_PDA for consistency (also with the TS SDK types)
renamed verifyQueryResponseSignatures to verifyQueryResponse since it also establishes quorum
removed getResponseHash() and renamed synonymously named getResponseDigest() to calcPrefixedResponseHash()
renamed EthCallData to EthCallRecord to avoid ambiguity
renamed EthCall structs' result field to results to fix inconsistent plural between eth and solana structs
renamed blockTime to blockTimeInMicroSeconds and minBlockTime to minBlockTimeInSeconds
removed leading underscores of function parameter names
removed trailing spaces
deleted redundant/misleading/nonsensical comments
enforced max line length of 100 characters (except in tests)

@nonergodic nonergodic changed the base branch from main to major-cleanup-no-ccq September 2, 2024 20:27
Base automatically changed from major-cleanup-no-ccq to main September 3, 2024 00:39
@nonergodic nonergodic marked this pull request as ready for review October 30, 2024 00:37
@nik-suri nik-suri requested review from djb15 and bruce-riley November 7, 2024 23:44
src/libraries/QueryResponse.sol Show resolved Hide resolved
src/libraries/QueryResponse.sol Show resolved Hide resolved
src/libraries/QueryResponse.sol Show resolved Hide resolved
Copy link

@djb15 djb15 left a comment

Choose a reason for hiding this comment

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

Not a full security review, but the changes generally look good to me!

I glanced through most of the methods that I believe just contain cosmetic changes, but I spent some time on the signature verification and it looks sensible + more gas efficient than before

@nonergodic nonergodic merged commit 65f905f into main Nov 14, 2024
1 check passed
@nonergodic nonergodic deleted the fix-ccq branch November 14, 2024 01:13
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.

3 participants