Skip to content

Conversation

@nguyer
Copy link
Contributor

@nguyer nguyer commented Nov 7, 2022

Signed-off-by: Nicko Guyer nicko.guyer@kaleido.io

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
@codecov-commenter
Copy link

Codecov Report

Merging #1095 (d2efbf1) into main (9bbec3b) will increase coverage by 0.00%.
The diff coverage is 98.58%.

@@           Coverage Diff            @@
##             main    #1095    +/-   ##
========================================
  Coverage   99.98%   99.98%            
========================================
  Files         307      308     +1     
  Lines       20331    20467   +136     
========================================
+ Hits        20327    20463   +136     
- Misses          3        4     +1     
+ Partials        1        0     -1     
Impacted Files Coverage Δ
internal/apiserver/routes.go 100.00% <ø> (ø)
internal/metrics/metrics.go 97.43% <0.00%> (-2.57%) ⬇️
pkg/core/contracts.go 100.00% <ø> (ø)
pkg/core/event.go 100.00% <ø> (ø)
pkg/core/operation.go 100.00% <ø> (ø)
pkg/core/transaction.go 100.00% <ø> (ø)
internal/apiserver/route_post_contract_deploy.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 3 more

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

return err
}

client := e.fftmClient
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is redundant now I believe

Copy link
Contributor

@peterbroadhurst peterbroadhurst left a comment

Choose a reason for hiding this comment

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

Looks great.

I know we've discussed a follow-up, to detect if EthConnect returns an error due to not supporting this spelling of the ContractDeploy function.

Two additional work items I noticed in the review:

  1. Directly related to change: Adding the new event types to this table: https://github.com/hyperledger/firefly/blob/1bb1675b41ef80875da870ae6b738d675c136364/docs/reference/types/_includes/event_description.md#reference-
  2. Unrelated cruft added to by this PR: Remove the defunct fftm configuration, from early in the v1.1 cycle when we had an option for a 2nd URL as not everything had moved to the new connector architecture:
    if fftmConf.GetString(ffresty.HTTPConfigURL) != "" {
    e.fftmClient = ffresty.New(e.ctx, fftmConf)
    }

@peterbroadhurst peterbroadhurst merged commit 981f3ed into hyperledger:main Nov 8, 2022
@peterbroadhurst peterbroadhurst deleted the deploy-contract branch November 8, 2022 01:55
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.

3 participants