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 token pool "confirmed" state with "active" bool #1305

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

awrichar
Copy link
Contributor

@awrichar awrichar commented May 17, 2023

Token pool states were originally given similar names to message states, but this has ended up being confusing (because the token pool creation flow also includes a message, but the token pool states have no relation to the identically named message states).

Old terminology:
When pool definition message is "confirmed", pool is created as "pending"
When pool gets "activated" by the connector, pool is moved to "confirmed"

New proposed terminology:
When pool definition message is "confirmed", pool is created as "not active"
When pool gets "activated" by the connector, pool is moved to "active"

My hope is that the new terminology is easier to follow, since it does not overlap with messaging states and more clearly denotes the action of "activating" a token pool. Also with #1261 in the works (which means a pool may get activated first and then later be published by a broadcast message), I think it's better to separate the token pool and message terminology.

This is technically a breaking API change, although I'm not aware of any significant usefulness of the "state" field to
external applications (it's primarily an internal tracking field). The related event called "token_pool_confirmed" will
remain unchanged - it's still consistent with other definitions like datatypes, identities, etc, and I think it's still
appropriate.

@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Merging #1305 (0890a10) into main (56b8d2f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1305   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          310       310           
  Lines        20960     20960           
=========================================
  Hits         20960     20960           
Impacted Files Coverage Δ
pkg/core/tokenpool.go 100.00% <ø> (ø)
internal/assets/token_approval.go 100.00% <100.00%> (ø)
internal/assets/token_transfer.go 100.00% <100.00%> (ø)
internal/database/sqlcommon/tokenpool_sql.go 100.00% <100.00%> (ø)
internal/definitions/handler_tokenpool.go 100.00% <100.00%> (ø)
internal/definitions/sender_tokenpool.go 100.00% <100.00%> (ø)
internal/events/token_pool_created.go 100.00% <100.00%> (ø)
internal/orchestrator/txn_status.go 100.00% <100.00%> (ø)
internal/reference/reference.go 100.00% <100.00%> (ø)

pkg/core/tokenpool.go Outdated Show resolved Hide resolved
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.

I like the name change. I do have concerns about it not being backwards compatible though.

@awrichar
Copy link
Contributor Author

I think #1261 will be the bigger backwards-incompatible change in the next release... so might as well rope them together while you have to migrate your token pool code anyway, right?

Token pool states were originally given similar names to message state, but
this has ended up being confusing (because the token pool creation flow
includes a message, but the token pool states have no relation to the
identically named message states).

Old terminology:
When pool definition message is "confirmed", pool is created as "pending"
When pool gets "activated" by the connector, pool is moved to "confirmed"

New terminology:
When pool definition message is "confirmed", pool is created as "not active"
When pool gets "activated" by the connector, pool is moved to "active"

My hope is that the new terminology is easier to follow, since it does not
overlap with messaging states and more clearly denotes the action of
"activating" a token pool.

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

Few thoughts:

  • I think Inactive better than Not Active
  • A diagram of the state machine might be helpful (eventually in the docs), including the states before the publish is perform, and after that point

@awrichar
Copy link
Contributor Author

@peterbroadhurst I guess my comment made it seem like the state was called Not Active. It's a bool as I've proposed it here, so it's either Active or not Active. I can draw a picture too.

@nguyer nguyer merged commit 16c6981 into hyperledger:main Sep 14, 2023
14 checks passed
@nguyer nguyer deleted the activation branch September 14, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants