uses an empty array to represent a null filter parameter

[gordienoye]

Because FilterOptions defines topics

final List<List<String>>? topics;

there is no easy way to pass in null as a topic filter. This is not an issue if the "match all" topic is at the end of the list of topics; but, if the "match all" topic is not the last topic and null needs to be passed when creating a FilterOptions object there needs to be an explicit way of specifying "match all".

This change uses an empty array [] to represent null and does the translation between [] and null when _createParamsObject is called. Since a dynamic array is returned this allows the insertion of a null element in the array, something that is not possible when creating the FilterOptions object.

---

NOTE: This change limits the alterations to the existing code base. It may make better architectural sense to make a change to FilterOptions to allow topics in the array to be explicitly null (and potentially to be of type String) instead of each being an array of strings. ... and I may not have understood how to use filter correctly in the first place.

[simolus3]

This looks good to me, but I agree that we may also want to make it a List<List<String>?>?.

[gordienoye]

@simolus3 I added a change to remove the two "unnecessary imports" errors that came from the static analysis stage and blocked the ability to merge.

Run dart analyze --fatal-infos
Analyzing web3dart...

   info - lib/src/browser/credentials.dart:9:8 - The import of '../../credentials.dart' is unnecessary because all of the used elements are also provided by the import of 'package:web3dart/web3dart.dart'. Try removing the import directive. - unnecessary_import
   info - lib/src/contracts/generated_contract.dart:5:8 - The import of 'deployed_contract.dart' is unnecessary because all of the used elements are also provided by the import of '../../web3dart.dart'. Try removing the import directive. - unnecessary_import

2 issues found.

[gordienoye]

@simolus3 I have been beating my head against the wall trying to figure out how to get a unit test into the code I changed. I am not a Dart expert but I cannot get a test in the private methods and then see the results of the test. Even if I inject the filter parameters I cannot see the result that is sent to the blockchain.

If you have any advice or direction, I would appreciate it.

[simolus3]

Currently, there are no unit tests for RPC behavior. This is precisely for the reason you mentioned (it's a bit complicated to set up). There are some integration tests with ganache, but that's it. This obviously isn't great, so I appreciate you asking about this!

In fa276faa98a7f745f54342d8cfa861e0f65a39b9, I have added a simple test for an RPC call that mocks an ethereum node internally. If you want to, you can take inspiration from that to ensure that registering a filter will call the corresponding method with the right arguments.

[codecov-commenter]

Codecov Report

Merging #228 (e06482a) into master (fa276fa) will decrease coverage by 0.83%. The diff coverage is 97.18%.

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   71.66%   70.82%   -0.84%     
==========================================
  Files          55       56       +1     
  Lines        2213     2283      +70     
==========================================
+ Hits         1586     1617      +31     
- Misses        627      666      +39     

Impacted Files	Coverage Δ
lib/src/contracts/generated_contract.dart	0.00% <ø> (-42.86%)	:arrow_down:
test/mock_client.dart	32.35% <83.33%> (-44.92%)	:arrow_down:
lib/src/core/filters.dart	31.64% <100.00%> (+9.85%)	:arrow_up:
test/core/event_filter_test.dart	100.00% <100.00%> (ø)
test/infura_integration_test.dart	53.33% <0.00%> (-40.01%)	:arrow_down:
lib/src/generated/erc20.g.dart	0.00% <0.00%> (-15.52%)	:arrow_down:
lib/src/contracts/deployed_contract.dart	0.00% <0.00%> (-10.00%)	:arrow_down:
lib/src/contracts/abi/abi.dart	88.88% <0.00%> (-5.56%)	:arrow_down:
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fa276fa...e06482a. Read the comment docs.

[gordienoye]

@simolus3 I would appreciate being able to leave this in your hands at this point. The code changes have been covered with unit tests but the mock client and some files outside these changes (afaik) are causing the overall project coverage to drop and thus block me merging the change.

If the overall change does not work for you, I might be able to take a look at going through the code in more detail a bit later to change the empty List into allowing null explicitly as a parameter when specifying a filter. I am just a bit pressed for time right now so this would likely be in a month or so.

[simolus3]

Actually it all looks good to me. The coverage bot is a bit weird unfortunately, but I think this is ready to get merged.

[simolus3]

Thanks for your contribution!