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

feat: implement EboActor.onResponseProposed handler #19

Merged
merged 17 commits into from
Aug 9, 2024

Conversation

0xyaco
Copy link
Collaborator

@0xyaco 0xyaco commented Aug 8, 2024

🤖 Linear

Closes GRT-82

Description

  • Implemented EboActor.onResponseProposed handler
  • Extracted some shared behavior inside EboActor to private methods
  • Separated EboActor test suite inside a folder eboActor/ with a single file per method, as the set up/mocking for each handler + scenario tends to be pretty loaded
  • Mocked logger in some test files that were generating some stdout noise during vitest execution

Copy link

linear bot commented Aug 8, 2024

GRT-82 Implement onResponseProposed

We want the actors to handle the event ResponseProposed from prophet Orcale.

This handler should:

  • Update RequestActivity
  • Validate if the proposal was correct, Leveraging BlockNumberService
  • Dispute if proposal was not valid
  • Handle the case in which a dispute was already created.

Event: https://github.com/defi-wonderland/prophet-core/blob/d01bc1a0fe39d516ff502a669e80b8eb2fe2df86/solidity/interfaces/IOracle.sol#L29

Follow figma sequence diagram: https://www.figma.com/board/BbciqJb5spg35ZglTsRRBb/Offchain?node-id=239-2711&t=y06FKcx0NVro4Jml-4
AC:

  • Event handler successfully implemented
  • Unit tests

@0xyaco 0xyaco requested review from 0xkenj1 and 0xnigir1 August 8, 2024 00:27
Copy link
Collaborator

@0xkenj1 0xkenj1 left a comment

Choose a reason for hiding this comment

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

💯

Comment on lines 125 to 126
const { currentEpoch, currentEpochTimestamp } =
await this.protocolProvider.getCurrentEpoch();
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think there is no need to call this in here, probably handle the epoch from the EboProcessor is better to avoid extra calls. EboActor should already know the epoch on creation time, wdyt ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also if we decide to call this here, lets save it on the attributes from the instance

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Delightful comment; yes, the EboActor will always handle the same epoch so these values can be passed within the constructor 💯

Comment on lines 188 to 190
const request = this.registry.getRequest(requestId);

if (!request) {
Copy link
Collaborator

@0xkenj1 0xkenj1 Aug 8, 2024

Choose a reason for hiding this comment

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

Suggested change
const request = this.registry.getRequest(requestId);
if (!request) {
if (requestId.toLowerCase() !== this.requestId.toLowerCase() ) {

},
});

vi.spyOn(registry, "getRequest").mockReturnValue(undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be changed if we decide to simplify the requestId check on the previous comment

Base automatically changed from feat/on-request-created to dev August 8, 2024 18:11
@0xyaco 0xyaco requested a review from 0xkenj1 August 8, 2024 19:01
Comment on lines +19 to +23
private readonly actorRequest: {
id: string;
epoch: bigint;
epochTimestamp: bigint;
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

noiiiiceeeeeee

Copy link
Collaborator

@0xnigir1 0xnigir1 left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

@0xyaco 0xyaco merged commit 647f044 into dev Aug 9, 2024
5 checks passed
@0xyaco 0xyaco deleted the feat/on-response-proposed branch August 9, 2024 13:16
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