Skip to content

Rename contract subscriptions to contract listeners #548

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Mar 3, 2022
Merged

Rename contract subscriptions to contract listeners #548

merged 4 commits into from
Mar 3, 2022

Conversation

@nguyer
Copy link
Contributor

@nguyer nguyer commented Feb 24, 2022 •
edited
Loading

This PR includes several noteworthy changes:

  • Contract Subscriptions have now been renamed to Contract Listeners everywhere, to reduce confusion. This includes the /contracts/subscriptions endpoint. It is now /contracts/listeners
  • The /subscribe endpoints on Contract Interfaces and APIs have been dropped
  • The /contracts/listeners endpoint can now take a interface reference to point it at an existing FFI, rather than putting the full FFI for the event inline
  • blockchain_event has been renamed to blockchain_event_received
  • The FFI generator endpoint for Ethereum now expects the ABI nested one level deeper. The payload should now look like:
{
  "input": {
    "abi": []
  }
}

peterbroadhurst
peterbroadhurst reviewed Mar 1, 2022
View reviewed changes
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.

Note I think we should:

  • Coordinate a merge of #549 with this, as anyone who up-migrates will be unable to apply that after (it contains the missing gaps)
  • Consider shipping a v0.13.1 before we merge this (or #549 or #517) as I think all those would need 2nd digit bump

peterbroadhurst
peterbroadhurst requested changes Mar 1, 2022
View reviewed changes
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.

Changes look great. Some very minor suggestions, and a comment on coordinating the merge.

@peterbroadhurst peterbroadhurst mentioned this pull request Mar 1, 2022
Merged
peterbroadhurst
peterbroadhurst reviewed Mar 2, 2022
View reviewed changes
internal/blockchain/ethereum/ethereum.go
ID string `json:"id,omitempty"`
}

type FFIGenerationInput struct {
Copy link
Contributor

@peterbroadhurst peterbroadhurst Mar 2, 2022

Choose a reason for hiding this comment

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

Is this change described in the PR description?

It has migration implications.

Copy link
Contributor Author

@nguyer nguyer Mar 2, 2022

Choose a reason for hiding this comment

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

Thanks I will note that. This is a breaking change which should be called out in the release notes as well.

peterbroadhurst
peterbroadhurst approved these changes Mar 2, 2022
View reviewed changes
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.

@nguyer - thanks for the updates. One additional comment to make sure we document the abi nesting change.

@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2022 •
edited
Loading

Codecov Report

Merging #548 (94df600) into main (25d5800) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #548   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          297       295    -2     
  Lines        17036     16997   -39     
=========================================
- Hits         17036     16997   -39
Impacted Files Coverage Δ
internal/oapiffi/ffi2swagger.go 100.00% <ø> (ø)
pkg/fftypes/contract_listener.go 100.00% <ø> (ø)
pkg/fftypes/event.go 100.00% <ø> (ø)
...al/apiserver/route_delete_contract_subscription.go 100.00% <100.00%> (ø)
...erver/route_get_contract_listener_by_name_or_id.go 100.00% <100.00%> (ø)
internal/apiserver/route_get_contract_listeners.go 100.00% <100.00%> (ø)
...rnal/apiserver/route_post_new_contract_listener.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

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 25d5800...94df600. Read the comment docs.

@nguyer nguyer merged commit 4c29266 into hyperledger:main Mar 3, 2022
@nguyer nguyer deleted the contract-listeners branch March 3, 2022 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peterbroadhurst peterbroadhurst peterbroadhurst approved these changes

@awrichar awrichar Awaiting requested review from awrichar

@nickgaski nickgaski Awaiting requested review from nickgaski

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nguyer @codecov-commenter @peterbroadhurst