inject publickeyring #3086
inject publickeyring #3086
Conversation
There was a problem hiding this comment.
utACK
Don't know how the 'mergers' think about this but wouldn't it be cleaner if you rebase this on your previous PR (this PR doesn't only contain the inject publickeyring changes) ? You could add a 'blocked by #123' comment so it doesn't get merged before the parent. Would make for cleaner diffs
| public void canInstantiate() { | ||
| P2PService p2PService = mock(P2PService.class); | ||
| when(p2PService.getNumConnectedPeers()).thenReturn(new SimpleIntegerProperty(0)); | ||
| Storage storage = mock(Storage.class); |
There was a problem hiding this comment.
why is storage mocked separately? It's just passed as an argument like the rest.
There was a problem hiding this comment.
yeah that test is just a stub that got me thinking about the pubkey.
|
|
||
| import org.bitcoinj.core.Coin; | ||
|
|
||
| public class MakerFeeMaker { |
There was a problem hiding this comment.
not sure about the name, too many makers making things :)
Don't have any good alternatives though... MakerFeeFactory, MakerFeeCalculation, MakerFeeCreator, ... (if you don't find anything better don't worry)
This is just a wrapper for one of the util classes, so will you need to do this for every method in every Util class? Are you trying to avoid calls to static methods for unit testing purposes?
There was a problem hiding this comment.
I'm trying to get rid of the static util classes, for testing purpose but also for architectural purposes. this is part of the parent pr, see the description of this pr
i can only rebase this pr once the other one is in master. and then i will rebase it and it will be much smaller. |
| @@ -96,6 +98,7 @@ protected void configure() { | |||
| install(alertModule()); | |||
| install(filterModule()); | |||
| install(corePresentationModule()); | |||
| bind(PubKeyRing.class).toProvider(PubKeyRingProvider.class); | |||
There was a problem hiding this comment.
Is this guaranteed to be in singleton context?
Can you move it to the other bindings above, so th einstall calls are not mixed with bind calls?
There was a problem hiding this comment.
Ah from the test below its seems its singleton.
There was a problem hiding this comment.
yes i made sure that it is.
| @@ -42,7 +42,7 @@ | |||
| @Slf4j | |||
| @EqualsAndHashCode | |||
| @Getter | |||
| public final class PubKeyRing implements NetworkPayload, UsedForTradeContractJson { | |||
| public class PubKeyRing implements NetworkPayload, UsedForTradeContractJson { | |||
There was a problem hiding this comment.
Might be used in p2p messages so must be final. I know its prob. not used now but further refactoring might use it... all NetworkPayload classes should be final.
We use the term provider in a wider range. IMO its not bad, better then factory IMO as thats more overloaded. |
christophsturm
force-pushed
the
p2ptest
branch
from
291214d to
5579c30
Compare
| @@ -106,7 +106,7 @@ | |||
| private final TxFeeEstimationService txFeeEstimationService; | |||
| private final ReferralIdService referralIdService; | |||
| private final BSFormatter btcFormatter; | |||
| private MakerFeeMaker makerFeeMaker; | |||
| private MakerFeeProvider makerFeeProvider; | |||
based on #3081