Allow TransactionBuilder to accept `null` for `fee`

[barnjamin]

Problem

I'd like to rely on suggested params to populate most of the fields but when overriding the fee on the transaction with a flatFee the builder errors since both fee and flatFee are set.

If I try to pass null to the fee method, it only works in the case of BigInteger which may be changed at some point.

Solution

Allow fee method in builder to accept null for Integer or Long so we can properly unset the fee parameter while still allowing suggestedParameters to be passed instead of individual arguments.

[subhakundu]

Is someone working on this issue? I would like to work on it if no one is taking a look. This is my first time working on the code base. Please let me know the things I should be keeping in mind.

[jannotti]

No one is working on it, but it seems like a nice convenience. Go for it!

[subhakundu]

I have a few questions to ask before I raise the PR

1. When we are passing null to BigInteger one (code), we are setting fee as null. I am assuming behaviour will be the same for Long and Integer one as well. In that case, we just need to introduce a null check and set null when null is passed to Long or Integer method. Something like this

    public T fee(Integer fee) {
        if (fee != null) {
            if (fee < 0) throw new IllegalArgumentException("fee cannot be a negative value");
            this.fee = BigInteger.valueOf(fee);
        } else {
            this.fee = null;
        }
        return (T) this;
    }

2. Also, I am planning to write tests in TestTransaction.java. Because of my lack of experience in code, it would be great if someone could tell me what type of tests I need to perform. I don't want to miss any cases. If there is any other package which is dependent on this codebase, please let me know.

If above approach in (1) looks fine to you, I will raise PR soon. Also, please let me know if you have any feedbacks.

[jannotti]

This looks like the right approach to me. I don't know much about the Java sdk tests, but I'll try to look later. For me, as long as you have a test that sets the fee to null with an Integer and a Long argument, I think you're fine.

Separately, I wonder if another thing we should do is null out flatFee whenever fee is set and vice versa. @jasonpaulos do you see any reason not to do that?

[subhakundu]

Thanks @jannotti for the pointers. Please let me know if I should make flatfee related change in this PR. Or I can create a separate PR for that.

[subhakundu]

Also, I see got flatfee, we have null issue for Long and Integer. Should we apply same changes for flatfee as well? I am not sure if similar check is required firstValid or lastValid, but I see similar usage as well.

[jasonpaulos]

Separately, I wonder if another thing we should do is null out flatFee whenever fee is set and vice versa.

@jannotti yes this makes sense to me, since the values are mutually exclusive.

Also, I see got flatfee, we have null issue for Long and Integer. Should we apply same changes for flatfee as well?

@subhakundu yes I think flatFee should accept null values just like fee.

I am not sure if similar check is required firstValid or lastValid, but I see similar usage as well.

In my opinion it wouldn't be harmful to allow nulls for firstValid and lastValid, but I don't think there's any real benefit to it either. So I don't have a strong opinion here.

[subhakundu]

Thanks for the information. Then I am making following changes

1. Allowing fee and flatfee Long and Integer method to accept null values.
2. Setting flatfee to null if free is set.
3. I am not touching firstValud and lastValid.

I will try to keep same PR with the two changes. If it takes time, I might raise separate PRs.

[subhakundu]

I have added a PR addressing the changes discussed. Fee and flatfee is already being set to null as per the current code. I have added a unit test for better documentation of different scenarios. Please let me know your feedback and concerns.