Skip to content
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.

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

Replace namespace.remotename with namespace.multiparty.networkname #947

Merged
merged 6 commits into from
Aug 26, 2022

Conversation

awrichar
Copy link
Contributor

@awrichar awrichar commented Aug 10, 2022

This is a proposed rename for discussion.

Currently a namespace can have a name and a remoteName - name is how it's known locally (on APIs and most database objects), while remoteName is how it's written into objects that transmit the namespace in a multi-party system (specifically messages, batches, and groups on dataexchange, and the BatchPin method of the V1 blockchain contract).

Since the thing we've called remoteName is entirely specific to multiparty networks, it could be renamed to multiparty.networkNamespace, which seems more accurate. It would still default to the value of the local namespace name if unset.

Counterpoints:

  • These fields will continue to show as localNamespace and namespace on messages and groups when queried (and the latter can't easily be modified due to the way hashes are computed).
  • Tokens plugins have a similar name and remoteName (one is stored locally and one is sent in messages), so this would be a departure from that terminology. Update: decided to rename the tokens plugin remoteName to broadcastName based on comments below.

This is only relevant in the context of a multiparty network.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
@codecov-commenter
Copy link

Codecov Report

Merging #947 (0ec5b9e) into main (0dae452) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #947   +/-   ##
=======================================
  Coverage   99.97%   99.97%           
=======================================
  Files         299      299           
  Lines       19478    19479    +1     
=======================================
+ Hits        19473    19474    +1     
  Misses          4        4           
  Partials        1        1           
Impacted Files Coverage Δ
internal/coreconfig/coreconfig.go 100.00% <ø> (ø)
pkg/core/namespace.go 100.00% <ø> (ø)
internal/blockchain/common/common.go 100.00% <100.00%> (ø)
internal/blockchain/ethereum/ethereum.go 100.00% <100.00%> (ø)
internal/blockchain/fabric/fabric.go 100.00% <100.00%> (ø)
internal/broadcast/manager.go 100.00% <100.00%> (ø)
internal/broadcast/message.go 100.00% <100.00%> (ø)
internal/broadcast/operations.go 100.00% <100.00%> (ø)
internal/data/blobstore.go 100.00% <100.00%> (ø)
internal/data/data_manager.go 100.00% <100.00%> (ø)
... and 39 more

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

@peterbroadhurst
Copy link
Contributor

I like multiparty.networkNamespace 👍

As the field is called namespace in all Multi-party data structures, calling it just multiparty.networkName seems like it might confuse things.

On the Tokens front, that's an interesting separate conversation. Because it's a name specific to the combination of a Token connector, and how it should be broadcast within a multiparty system. I wonder if broadcastName (rather than remoteName) might make sense there.

@awrichar awrichar added this to the v1.1.0 milestone Aug 19, 2022
@awrichar awrichar self-assigned this Aug 19, 2022
@awrichar
Copy link
Contributor Author

Yea, I like that suggestion. Will update the PR.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
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.

I think this is a real positive step forwards in the terminology.

@awrichar awrichar merged commit c5cc7ed into hyperledger:main Aug 26, 2022
@awrichar awrichar deleted the networknamespace branch August 26, 2022 14:25
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