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

[BCI-3989][core] - CR methods err when service unstarted #14123

Draft
wants to merge 80 commits into
base: develop
Choose a base branch
from

Conversation

Farber98
Copy link
Contributor

@Farber98 Farber98 commented Aug 14, 2024

Task Description:

CR methods should throw an error if service is unstarted

This PR:

  • Adds StartChainReader and CloseChainReader methods to the EVM ChainReaderInterfaceTester.
  • Check if the service is already started before calling GetLatestValue, BatchGetLatestValues, and QueryKey
    • Didn't add the check to Bind, we can discuss later if it's needed there too.
    • Added an error case for each of the methods when calling without starting the service previously.
  • Add Start() call to consumers of CR before using it.
  • Modify EVM tests to include StartChainReader and CloseChainReader calls

Ticket:

Depends on:

commit cb25d0b
Author: Cedric Cordenier <[email protected]>
Date:   Wed Aug 14 11:00:55 2024 +0100

    Add enum for CapabilityType

commit 6470106
Author: Cedric Cordenier <[email protected]>
Date:   Wed Aug 14 09:32:47 2024 +0100

    Update common

commit bbb11bd
Merge: 5265503 3399dd6
Author: Cedric <[email protected]>
Date:   Wed Aug 14 09:33:34 2024 +0100

    Merge branch 'develop' into KS-426-remove-panic-capability-type

commit 5265503
Author: Cedric Cordenier <[email protected]>
Date:   Wed Aug 14 09:32:47 2024 +0100

    Update common

commit 3e1cc15
Merge: a80942f eb31cf7
Author: Vyzaldy Sanchez <[email protected]>
Date:   Tue Aug 13 11:25:22 2024 -0400

    Merge branch 'develop' into KS-426-remove-panic-capability-type

commit a80942f
Author: Cedric Cordenier <[email protected]>
Date:   Mon Aug 12 14:14:24 2024 +0100

    [chore] Remove panic from CapabilityType type
@Farber98
Copy link
Contributor Author

Farber98 commented Aug 15, 2024

Still need to

  • Improve tests checking errors when calling the methods without the service started
  • Find a workaround and prevent the breaking change + multiple repo PR

EDIT:

  • done
  • couldn't

@Farber98 Farber98 changed the title add isStarted check [BCI-3989][core] - CR methods err when service unstarted Aug 16, 2024
@Farber98 Farber98 requested a review from a team as a code owner September 3, 2024 14:28
@Farber98 Farber98 marked this pull request as draft September 12, 2024 18:43
@Farber98 Farber98 marked this pull request as ready for review September 13, 2024 11:15
@Farber98 Farber98 marked this pull request as draft September 13, 2024 22:46
@Farber98
Copy link
Contributor Author

Farber98 commented Sep 16, 2024

Refactored the structure of EVMInterfaceTester for clarity and to make the proposed solution work:

  • Setup receives the started flag to check if we should return CR started
  • Setup divided into small functions to make it easier to understand the purpose/responsibility of each one.
  • Get functions now only return the instance. Setup is handled separately

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.

5 participants