Skip to content

Conversation

@gitmp01
Copy link
Collaborator

@gitmp01 gitmp01 commented Jun 13, 2024

What to look:

  • Bugs in ProofcastAdapter.sol
  • Missing tests in test/adapters/Proofcast
  • Off chain event encoding in test/adapters/Proofcast/EventAttestator.ts

@gitmp01 gitmp01 requested a review from ubordignon June 13, 2024 08:52
if (yahos[domain] == address(0)) revert UnsupportedChainId(domain);

offset += 32; // skip the event id (32 bytes)
bytes memory eventBytes = statement[offset:];

Choose a reason for hiding this comment

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

From the statement format, you should have the event id at this offset.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isn't it skipped on line 95?

Choose a reason for hiding this comment

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

My bad, I overlooked offset += 32 at line 91.
Resolving this and the other comment, which was also tied to the fact that I didn't spot the event id skipping.


// MessageDispatched event parsing
address yahoAddress = RLPReader.toAddress(eventContent[0]);
if (yahoAddress != yahos[domain]) revert InvalidYahoAddress();

Choose a reason for hiding this comment

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

There's no test for this revert.

Choose a reason for hiding this comment

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

This is my last comment, everything else LGTM!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thank you, I'll sort it out in the afternoon

@gitmp01 gitmp01 force-pushed the feat/add-proofcast-adapter branch from a69d98f to 7244cd9 Compare June 17, 2024 16:40
error InvalidMessageId(uint256 actual, uint256 expected);
error InvalidDestinationChainId(uint256 chainId);
error GracePeriodNotElapsed();
error InvalidNewTeeSigner();

Choose a reason for hiding this comment

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

Out of completeness you could include a test for this new error type as well, although it is not necessary, as applyNewTeeSigner is very straight forward.

error UnsupportedProtocolId(bytes1 protocolId);
error UnsupportedChainId(uint256 chainId);
error UnexpectedEventTopic(bytes32 topic);
error InvalidSender();

Choose a reason for hiding this comment

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

Unused, can be remove.

error UnexpectedEventTopic(bytes32 topic);
error InvalidSender();
error InvalidMessageId(uint256 actual, uint256 expected);
error InvalidDestinationChainId(uint256 chainId);

Choose a reason for hiding this comment

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

Unused, can be remove.

import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
import { ECDSA } from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import { RLPReader } from "solidity-rlp/contracts/RLPReader.sol";
import { IERC777Recipient } from "@openzeppelin/contracts/interfaces/IERC777Recipient.sol";

Choose a reason for hiding this comment

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

Unused, can be remove.

import { ECDSA } from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import { RLPReader } from "solidity-rlp/contracts/RLPReader.sol";
import { IERC777Recipient } from "@openzeppelin/contracts/interfaces/IERC777Recipient.sol";
import { IERC1820RegistryUpgradeable } from "@openzeppelin/contracts-upgradeable/interfaces/IERC1820RegistryUpgradeable.sol";

Choose a reason for hiding this comment

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

Unused, can be remove.

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