Simplified and improved kotlin forwarding service

[ItoroD]

• One main difference to note in the kotlin version of forwarding service is that in kotlin its not the best best practice to use multiple constructors (secondary constructors). Hence default values have been used for some member variables of config class.
• With this we can initialize config as if it had multiple constructors

[msgilligan]

Overall, this looks great!

I am being exceptionally picky about the ForwardingService because for many it will be their first impression of bitcoinj (and maybe Kotlin or modern Java) and also because if we try really hard and can't eliminate any unwieldy code then we need to look at improving the bitcoinj API itself to fix them.

I really appreciate your work and your willingness to keep fine-tuning this!

[msgilligan]

Getting close! This is looking good!

[msgilligan]

We generally try to avoid using wildcard imports...

[ItoroD]

will look out those

[msgilligan]

Is this constructor necessary?

[ItoroD]

Yes, I noticed you do the address check in a second constructor that has (address, network) as parameters. So this constructor for when only address is passed, calls the other constructor that checks address before address is set on member variables.

since forwardingAddress.network() as BitcoinNetwork is now being done in the constructor, i should also take it out of the default set value for network

[ItoroD]

Overall, this looks great!

I am being exceptionally picky about the ForwardingService because for many it will be their first impression of bitcoinj (and maybe Kotlin or modern Java) and also because if we try really hard and can't eliminate any unwieldy code then we need to look at improving the bitcoinj API itself to fix them.

I really appreciate your work and your willingness to keep fine-tuning this!

Thanks! @msgilligan

[msgilligan]

Approved.

[schildbach]

I wonder can we do anything about these warnings as well?

w: file:///bitcoinj/examples-kotlin/src/main/kotlin/org/bitcoinj/ForwardingService.kt:94:81 Parameter 'prevBalance' is never used
w: file:///bitcoinj/examples-kotlin/src/main/kotlin/org/bitcoinj/ForwardingService.kt:94:100 Parameter 'newBalance' is never used

Obviously we can't change the method signature, but is it possible to declare the parameters as "intentionally not being used"?

[schildbach]

One nitpick about the commit message: we use present tense to describe the semantics of the commit, rather than past tense. So "simplify coin listener" rather than "simplified…".

[ItoroD]

One nitpick about the commit message: we use present tense to describe the semantics of the commit, rather than past tense. So "simplify coin listener" rather than "simplified…".

Noted. Thanks

[ItoroD]

I wonder can we do anything about these warnings as well?

w: file:///bitcoinj/examples-kotlin/src/main/kotlin/org/bitcoinj/ForwardingService.kt:94:81 Parameter 'prevBalance' is never used
w: file:///bitcoinj/examples-kotlin/src/main/kotlin/org/bitcoinj/ForwardingService.kt:94:100 Parameter 'newBalance' is never used

Obviously we can't change the method signature, but is it possible to declare the parameters as "intentionally not being used"?

We can use the "Suppress" annotation like so

@Suppress("UNUSED_PARAMETER")
private fun coinForwardingListener(wallet: Wallet, incomingTx: Transaction, prevBalance: Coin, newBalance: Coin)

[msgilligan]

Obviously we can't change the method signature, but is it possible to declare the parameters as "intentionally not being used"?

Can't they be replaced with _ or prefixed with _?

Note that this convention is now in Java 22 and later: https://openjdk.org/jeps/456

[ItoroD]

Obviously we can't change the method signature, but is it possible to declare the parameters as "intentionally not being used"?

Can't they be replaced with _ or prefixed with _?

Note that this convention is now in Java 22 and later: https://openjdk.org/jeps/456

Yes the underscore exists in kotlin but it cannot be used on parameter of a function. Its used in special cases like parameter of a lamda function.

Also in java 22 I doubt you can use the underscore in a method param. Its used in other cases in java 22 as well. (not in method params) to the best of my knowledge

[schildbach]

@msgilligan Are you fine with the added @Suppress? Or should we do without and live with the warnings?

[msgilligan]

@msgilligan Are you fine with the added @Suppress? Or should we do without and live with the warnings?

I'm fine with it for now. I spent some time looking at alternatives and while doing so discovered that the neither the Kotlin nor the Java example correctly removes the listener. The problem is that when creating the method references now objects are created. So the listener passed in to remove does not match the one stored in the list...

[schildbach]

Merged.