Starting structure and interface definition for txpool module

[lsebrie]

No description provided.

[colltoaction]

Don't inject config. Also consider using an @Component annotation when a @Bean is not necessary.

[colltoaction]

Prefer private final fields for dependencies: https://github.com/rsksmart/rskj/blob/contributing/CONTRIBUTING.md#java-styleguide

[colltoaction]

Maybe you can also be the first one to apply the Check not null preconditions styleguide!

[colltoaction]

Maybe it would be better to add a JavaDoc explaining the return values, instead of this comment.

[lsebrie]

👍

[colltoaction]

tpm?

[lsebrie]

👍

[colltoaction]

Maybe this won't scale with harder tests, but how about asserting the exact JSON string?

[lsebrie]

These tests weren't pretending to test real values, just a basic syntactic structure.

[diega]

There is a missing space before {

[lsebrie]

👍

[diega]

Maybe we can have something like this

public interface TxPoolWeb3Module {

    default String txpool_content() {
        return getTxPoolModule().content();
    }

    default String txpool_inspect() {
        return getTxPoolModule().inspect();
    }

    default String txpool_status() {
        return getTxPoolModule().inspect();
    }

    TxPoolModule getTxPoolModule();
}

and make org.ethereum.rpc.Web3Impl to implement it. Then this class should only implement the getTxPoolModule() returning the injected TxPoolModuleImpl field and we let the actual forwarding to happen inside the default methods of the interface

[colltoaction]

Interesting, but it makes me wonder whether that will work with jsonrpc4j mapping.
At the same time, this looks like a hack for our contrived architecture, but i'm not against it :)

[colltoaction]

Also, the getModule name would clash with others later on. How about:

public interface Web3 {

    default String txpool_content() {
        return getTxPoolModule().content();
    }

    default String txpool_inspect() {
        return getTxPoolModule().inspect();
    }

    default String txpool_status() {
        return getTxPoolModule().inspect();
    }

    TxPoolModule getTxPoolModule();

    public interface TxPoolModule {

        String content();

        String inspect();

        String status();
    }
}

My concern is that if we followed this pattern for all our modules, the Web3Impl class would do nothing. What would be the responsibility of this class?:

public class Web3Impl implements Web3 {
    public Web3Impl(Module1 m1, Module2 m2, ...) {
        this.m1 = m1;
        this.m2 = m2;
        ...
    }

    public Module1 M1 getModule1() {
        return this.m1;
    }

    public Module2 M2 getModule2() {
        return this.m2;
    }
    ...
}

[diega]

Yes, I edited the getModule method right after sending it :)
I think jsonrpc4j should still work b/c it uses reflection to resolve the method of Web3Impl to execute, but of course that will be the first test to run.
The only responsibility of this class will be to be useful in the context of jsonrpc4j, just that.

[lsebrie]

I like the idea about an interface with a default implementation that web3 require for modules. I don't think it's a good idea to tie both interfaces. I will add default implementation for txpool module. We can in other PR introduce them for others modules

[rskops]

SonarQube analysis reported 273 issues

• 1 blocker
• 162 major
• 93 minor
• 17 info

Top 10 issues

1. Web3Impl.java#L508: "blockchain" is a method parameter, and should not be used for synchronization.
2. RskFactory.java#L115: This block of commented-out lines of code should be removed.
3. RskFactory.java#L141: Method has 18 parameters, which is greater than 7 authorized.
4. Web3RskImpl.java#L62: Constructor has 18 parameters, which is greater than 7 authorized.
5. Web3RskImpl.java#L136: Change this "try" to a try-with-resources. (sonar.java.source not set. Assuming 7 or greater.)
6. Web3RskImpl.java#L155: Refactor this code to not nest more than 3 if/for/while/switch/try statements.
7. Web3RskImpl.java#L166: Either log or rethrow this exception.
8. Web3RskImpl.java#L166: Either remove or fill this block of code.
9. Web3.java#L32: Make startingBlock a static final constant or non-public and provide accessors if needed.
10. Web3.java#L33: Make currentBlock a static final constant or non-public and provide accessors if needed.