hyperledger / fabric-sdk-go

https://wiki.hyperledger.org/display/fabric
Apache License 2.0
911 stars 508 forks source link

fix: allow negative threshold for MinBlockHeight #195

Closed mblottiere closed 2 years ago

mblottiere commented 2 years ago

Negative values are used to disable the threshold and currently appear in unit tests and in preferorg's tests. We currently have a real use case for this.

My understanding is that there is no reason not to allow negative values, but I'd be glad to discuss this further.

I attempted to retain the default behavior, but I don't see how to do so without massive impact. We would need to be able to distinguish between the zero and the unset case, which is not currently possible with an int field in the policy. I quickly tried to switch to a string field but did not pursue this way as this would impact many other modules. So I went to find a middle ground: default is only applied when the resolver mode is not explicitly set and value is less or equal than zero. I'm not very happy with this since there are different behaviors depending on the resolver mode being set or not. On the other hand the lag threshold only have meaning when using the appropriate resolver, so I guess this is fine.

mblottiere commented 2 years ago

I noticed the failing test and will rework the patch to still retain the default height behavior.

codecov[bot] commented 2 years ago

Codecov Report

Merging #195 (29a8efe) into main (5c3f546) will increase coverage by 0.03%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
+ Coverage   76.11%   76.14%   +0.03%     
==========================================
  Files         193      193              
  Lines       14078    14075       -3     
==========================================
+ Hits        10716    10718       +2     
+ Misses       2394     2388       -6     
- Partials      968      969       +1     
Impacted Files Coverage Δ
.../events/client/peerresolver/minblockheight/opts.go 80.00% <ø> (+8.30%) :arrow_up:
...ection/dynamicselection/pgresolver/lbpolicyimpl.go 70.83% <0.00%> (ø)

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 5c3f546...29a8efe. Read the comment docs.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mblottiere commented 2 years ago

This is still relevant.