Skip to content

Conversation

@peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Jan 12, 2023

The address resolver is a very useful plug point for FireFly with EVM based chains that have sophisticated signing infrastructures.

We have a requirement for the interface to be called on every API call that has a key input on the Hyperledger FireFly API, regardless of whether the input string conforms to an 0x address already, and regardless of how many times it is called.

This PR introduces a single new config option alwaysResolve to do this.

I added a second commit on this PR, after doing quite a detailed review of every place we were calling the address
resolver. I found the terminology was still confusing (know we've had a few changes) and make a tweak.

I also made the following changes:

  • Removed two places within the ethereum connector it was using the NormalizeSigningKey function, to just format an ethereum address. That is now fixed
  • I removed the additional cache on address resolution that was in the Identity Manager, because this would have got int the way of the removal of the cache in the blockchain layer. Also, both Ethereum and Fabric already had caches for the expensive logic that wasn't just string formatting, so it seemed completely redundant.
  • Fixed an intermittent coverage gap in Webhooks, fixing also the fact we were logging an error when there wasn't one, and removing a return of an error from doDelivery that was always nil

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2023

Codecov Report

Merging #1141 (809f83f) into main (0697425) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              main    #1141      +/-   ##
===========================================
- Coverage   100.00%   99.98%   -0.02%     
===========================================
  Files          303      303              
  Lines        19799    19783      -16     
===========================================
- Hits         19799    19781      -18     
- Misses           0        1       +1     
- Partials         0        1       +1     
Impacted Files Coverage Δ
internal/coreconfig/coreconfig.go 100.00% <ø> (ø)
internal/apiserver/route_post_verifiers_resolve.go 100.00% <100.00%> (ø)
internal/assets/token_approval.go 100.00% <100.00%> (ø)
internal/assets/token_pool.go 100.00% <100.00%> (ø)
internal/assets/token_transfer.go 100.00% <100.00%> (ø)
internal/blockchain/ethereum/address_resolver.go 100.00% <100.00%> (ø)
internal/blockchain/ethereum/config.go 100.00% <100.00%> (ø)
internal/blockchain/ethereum/ethereum.go 100.00% <100.00%> (ø)
internal/blockchain/fabric/fabric.go 100.00% <100.00%> (ø)
internal/contracts/manager.go 100.00% <100.00%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst marked this pull request as ready for review January 13, 2023 04:38
@peterbroadhurst peterbroadhurst marked this pull request as draft January 13, 2023 04:45
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@peterbroadhurst peterbroadhurst marked this pull request as ready for review January 13, 2023 04:53
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@nguyer
Copy link
Contributor

nguyer commented Jan 13, 2023

I ran the tests on this branch locally and have verified that there is no longer a gap in coverage, despite what codecov is reporting.

Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Makes sense to me.

@nguyer nguyer merged commit 21873ba into main Jan 13, 2023
@nguyer nguyer deleted the always-resolve branch January 13, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants