Removing the memoize from the signature algorithm

[mamoralesiob]

PR description

This PR tries to avoid the use of Memoize when getting the SignatureAlgorithm, since it is already a singleton that can be obtained through the SignatureAlgorithmFactory class. But, after removing this Memoize use, when executing the tests I could check that Memoize was being used, not only to cache the SignatureAlgorithm, but also for not loading the SignatureAlgorithm in the class using it until it was required. Right now, without Memoize, and loading the SignatureAlgorithm in most classes in this way

private final SignatureAlgorithm SIGNATURE_ALGORITHM = SignatureAlgorithmFactory.getInstance();

The SignatureAlgorithm is loaded at building time, so it gets the default value:

 public static SignatureAlgorithm getInstance() {
    return instance != null
        ? instance
        : SignatureAlgorithmType.DEFAULT_SIGNATURE_ALGORITHM_TYPE.get();
  }  

What happens in tests is that the SignatureAlgorithm gets the default value instead of the value existing in the json genesis files, so I'm a bit afraid that this also happens in production environments, so I would like that someone could check whether this is the correct.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

• [ ] Checked out our contribution guidelines?
• [ ] Considered documentation and added the doc-change-required label to this PR if updates are required.
• [ ] Considered the changelog and included an update if required.
• [ ] For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

• [ ] spotless: ./gradlew spotlessApply
• [ ] unit tests: ./gradlew build
• [ ] acceptance tests: ./gradlew acceptanceTest
• [ ] integration tests: ./gradlew integrationTest
• [ ] reference tests: ./gradlew ethereum:referenceTests:referenceTests
• [ ] hive tests: Engine or other RPCs modified?

[AliZDev-v0]

Hi @mamoralesiob, I'm currently learning about this project and came across your PR as part of that process - hoping I can contribute meaningfully.

While reviewing usage examples of SignatureAlgorithmFactory in the codebase, I noticed that it typically requires initialization via SignatureAlgorithmFactory.setInstance(...).

Based on that, tests behavior seems correct, since no instance of SignatureAlgorithm was set

[mamoralesiob]

Hi @mamoralesiob, I'm currently learning about this project and came across your PR as part of that process - hoping I can contribute meaningfully.

While reviewing usage examples of SignatureAlgorithmFactory in the codebase, I noticed that it typically requires initialization via SignatureAlgorithmFactory.setInstance(...).

Based on that, tests behavior seems correct, since no instance of SignatureAlgorithm was set

Thank you, @AliZDev-v0, for your answer. The problem is that before removing Memoize when getting the instance, tests load genesis conf from json files, so all tests executed successfully. After removing Memoize, it seems that this doesn't happen anymore, so getInstance method is always returning the default signature algorithm type and, therefore, some tests fail. This is what I was asking about, since it seems that Memoize was making that getInstance method was executing after this json files were loaded.

[github-actions[bot]]

This pr is stale because it has been open for 30 days with no activity.

[macfarla]

@mamoralesiob there's now 2 ways in the Factory to get the default instance - can this be simplified

  public static void setDefaultInstance() {
    instance = SignatureAlgorithmType.createDefault().getInstance();
  }

and

  public static SignatureAlgorithm getInstance() {
    return instance != null
        ? instance
        : SignatureAlgorithmType.DEFAULT_SIGNATURE_ALGORITHM_TYPE.get();
  }

[macfarla]

What happens in tests is that the SignatureAlgorithm gets the default value instead of the value existing in the json genesis files, so I'm a bit afraid that this also happens in production environments, so I would like that someone could check whether this is the correct.

Are there specific tests that are failing for you? We do run the tests in parallel so we do want to make sure we're not introducing test flakiness

[mamoralesiob]

What happens in tests is that the SignatureAlgorithm gets the default value instead of the value existing in the json genesis files, so I'm a bit afraid that this also happens in production environments, so I would like that someone could check whether this is the correct.

Are there specific tests that are failing for you? We do run the tests in parallel so we do want to make sure we're not introducing test flakiness

Hi, @macfarla. Right now, in the branch I removed the use of Memoize, I get the 3 following errors:

OperatorSubCommandTest > shouldFailIfImportedKeysAreFromDifferentEllipticCurve() FAILED
    java.lang.AssertionError at OperatorSubCommandTest.java:470

OperatorSubCommandTest > shouldGenerateSECP256R1KeysWhenSetAsEcCurve() FAILED
    org.opentest4j.AssertionFailedError at OperatorSubCommandTest.java:482

OperatorSubCommandTest > shouldImportSecp256R1Keys() FAILED
    java.lang.AssertionError at OperatorSubCommandTest.java:413

869 tests completed, 3 failed, 2 skipped