Skip to content

Conversation

@peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Sep 15, 2021

Work in progress on https://github.com/hyperledger-labs/firefly/issues/187

Key proposals in this code so far - building with Unit Tests

  • New Identity Manager [Im] component
    • This is not a plugin, instead a consistent component to do all identity mapping
    • This would be expected to be where plugins would attach in the future
    • Implementation includes testing of the various matrix options
    • Implementation includes wiring this into Orchestrator
    • Implementation includes changing imports on calling packages
    • TODO (in this PR): Fix all the code that that currently calls Resolve on the now defunct Identity Interface
  • New fftypes.Identity sub-object
    • Intended to replace single author field with author+key fields
    • Pass pointer to this part of the struct to the new im.ResolveInputIdentity function
    • TODO (in this PR): Embed this into all objects that can contain identity
  • Stubbing out the Identity Interface [Ii] plugin for now
    • Still retained onchain key for config compatibility
    • Removed all methods from interface
    • Changed implementation to TBD for now in the code
  • Organizations and Nodes still use the signing identity
  • For now the intention is that Node.Owner and Organization.Identity remain the signing addresses
  • We should move these to be DIDs, but that will mean migration issues
  • Per https://github.com/hyperledger-labs/firefly/issues/187#issuecomment-919649333 we need to consider this before acting

The next step here is e2e testing, and working through various scenarios to test.
I have only done build+UT so far.

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

Codecov Report

Merging #192 (22c7847) into main (9841abd) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #192      +/-   ##
===========================================
+ Coverage   99.97%   100.00%   +0.02%     
===========================================
  Files         201       202       +1     
  Lines       10974     11084     +110     
===========================================
+ Hits        10971     11084     +113     
+ Misses          2         0       -2     
+ Partials        1         0       -1     
Impacted Files Coverage Δ
pkg/fftypes/batch.go 100.00% <ø> (ø)
pkg/fftypes/message.go 100.00% <ø> (ø)
internal/assets/manager.go 100.00% <100.00%> (ø)
internal/batch/batch_manager.go 100.00% <100.00%> (ø)
internal/batch/batch_processor.go 100.00% <100.00%> (ø)
internal/batchpin/batchpin.go 100.00% <100.00%> (ø)
internal/blockchain/ethereum/ethereum.go 100.00% <100.00%> (ø)
internal/broadcast/datatype.go 100.00% <100.00%> (ø)
internal/broadcast/definition.go 100.00% <100.00%> (ø)
internal/broadcast/manager.go 100.00% <100.00%> (ø)
... and 26 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 9841abd...22c7847. Read the comment docs.

Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Comment on lines 40 to 63
signingIdentity := org.Identity
if org.Parent != "" {
signingIdentity = org.Parent
parent, err := sh.database.GetOrganizationByIdentity(ctx, org.Parent)
if err != nil {
return false, err // We only return database errors
}
if parent == nil {
l.Warnf("Unable to process organization broadcast %s - parent identity not found: %s", msg.Header.ID, org.Parent)
return false, nil
}
}

id, err := sh.identity.Resolve(ctx, signingIdentity)
if err != nil {
l.Warnf("Unable to process organization broadcast %s - resolve identity failed: %s", msg.Header.ID, err)
return false, nil
}

if msg.Header.Author != id.OnChain {
l.Warnf("Unable to process organization broadcast %s - incorrect signature. Expected=%s Received=%s", msg.Header.ID, id.OnChain, msg.Header.Author)
return false, nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@peterbroadhurst I'm not sure if this is right. This block of code was giving me trouble so I had it commented out for most of time while I was working on this PR. My gut tells me it shouldn't be removed though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this check does need re-instating with a tweak. Doing that now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is back now - checking that in the case of a child org:

  • The parent org is known
  • The transaction is signed by the parent org identity

│ ├───┤ identity [Im] │ - Central identity management service across components
│ │ │ manager │ * Resolves API input identity + key combos (short names, formatting etc.)
│ │ │ │ * Resolves registered on-chain signing keys back to identities
│ │ └───────────────┘ * Integrates with Blockchain Interface and plugable Identity Interface (TBD)
Copy link
Contributor

Choose a reason for hiding this comment

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

🤩

@@ -0,0 +1,9 @@
ALTER TABLE batches ADD COLUMN "key" VARCHAR(1024);
UPDATE batches SET "key" = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no way to make this NOT NULL in sqlite due to the migration path, is there? 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's I conclusion I came to.

// <-received2
// <-changes2 // also expect database change events
// // TODO: validate created pool
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k, seems this needs to be solved before merge

Copy link
Contributor

Choose a reason for hiding this comment

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

If it makes sense for me to be the one that does this, I'm happy to stick a commit on the end after other elements are stabilized.

// Resolve the signing key
if inputKey != "" {
if cached := im.signingKeyCache.Get(inputKey); cached != nil {
cached.Extend(im.identityCacheTTL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to manually Extend each time we fetch? Shouldn't this be handled transparently by the LRU cache?

Copy link
Contributor Author

@peterbroadhurst peterbroadhurst Oct 4, 2021

Choose a reason for hiding this comment

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

I didn't find clear docs on this (the code you describe is elsewhere in FireFly).

Digging into the code here...
https://github.com/karlseguin/ccache/blob/ef4bd54683f611ad3df9ba84689234c78243c464/cache.go#L101-L117

Then here...
https://github.com/karlseguin/ccache/blob/ef4bd54683f611ad3df9ba84689234c78243c464/cache.go#L394-L410

It seems the behavior if you don't extend, is that it will end up in the purge list, for LRU deletion.
While Get alone will make it less likely to be the thing that's deleted, that's not the same as ensuring it's retained for a period after being explicitly accessed.

I guess my vote is to leave the pattern as it is for the moment in this PR, then consider a wrapper around the caching framework in a separate PR that handles optimization of the logic... based on some testing. Maybe that means removing use of expiry altogether? Or maybe only using expiry in the case of a cache miss (kind of as proposed here I think).

cached.Extend(im.identityCacheTTL)
outputKey = cached.Value().(string)
} else {
outputKey, err = im.blockchain.ResolveSigningKey(ctx, inputKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this resolution always goes to the blockchain plugin. In the context of tokens, I'm wondering how identities need to be expressed. For instance, as of #211, pool creation includes an operation to the token connector and an operation to the blockchain connector. It's not guaranteed that these connectors are talking to the same blockchain, so is it possible we'd need to resolve two keys via the two plugins? Maybe a question to ponder with @peterbroadhurst...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think this is incompletely thought through for Tokens in this PR.
My suggestion is to address the token specifics in #215

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, fine by me

pool.Author = config.GetString(config.OrgIdentity)
}
author, err := am.identity.Resolve(ctx, pool.Author)
err := am.identity.ResolveInputIdentity(ctx, &pool.Identity)
Copy link
Contributor

Choose a reason for hiding this comment

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

I noted this in the identity code below too, but feels like we're ultimately going to need to support 2 "author" identities here (1 for the token pool connector and 1 for the blockchain). They're likely to be the same in simple cases (ie Ethereum+ERC1155), but we'll need to allow them to be specified differently in the input payload if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Let's accept that this PR leaves things in a "still broken" state before+after for tokens, and address in #215 after merge.


func (h *HTTPS) CreateTokenPool(ctx context.Context, identity *fftypes.Identity, pool *fftypes.TokenPool) error {
func (h *HTTPS) CreateTokenPool(ctx context.Context, pool *fftypes.TokenPool) error {
data := createPoolData{
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mostly a note to myself, but this is where the resolved key for the token connector needs to propagate into the REST call. Opened hyperledger/firefly-tokens-erc1155#30 to hopefully enable that to happen.

log.L(ctx).Errorf("Invalid signing identity '%s': %s", batch.Author, err)
return err
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's now assumed that the identity has been pre-verified before invoking this helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It comes from the messages in the batch which must each have been resolved, and will be verified by all members processing the batch, so doing an additional pre-submission check here does not seem efficient.

)

// SystemCore specifies an interface for global utility functions, without creating a cycle between components
type SystemCore interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see where this is used...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed 👍

}

func (bm *broadcastManager) BroadcastMessageWithID(ctx context.Context, ns string, id *fftypes.UUID, unresolved *fftypes.MessageInOut, resolved *fftypes.Message, waitConfirm bool) (out *fftypes.Message, err error) {
func (bm *broadcastManager) BroadcastMessageWithResolvedID(ctx context.Context, ns string, in *fftypes.MessageInOut, waitConfirm bool) (out *fftypes.Message, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't find where this method is used...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, pulling it out. Think this became unnecessary in some of the updates @nguyer made

resolved.Header.TxType = fftypes.TransactionTypeBatchPin
}

if !identityResolved {
Copy link
Contributor

@awrichar awrichar Oct 1, 2021

Choose a reason for hiding this comment

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

Personally I don't like when bits of a method are enabled/disabled with a boolean flag - feels like it should just be a separate method that you invoke beforehand if you need it.

Side note, after this block executes, the identity is resolved, right? So it's odd that the boolean is propagated on line 75 & 86 rather than being flipped to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think this is because the BroadcastRootOrgDefinition method was separated out, which is missing coverage.
Addressing this.

}

// THIS IS WHERE THE NEW CODE FIRST CALLS ResolveInputIdentity
// IT already has an author and key here
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment feels like a note to the developer and maybe not something that should be committed... or at least it should be cleaned up before being committed.

if len(nextPins) == 0 {
// If this is the first time we've seen the context, then this message is read as long as it is
// the first (nonce=0) message on the context, for one of the members, and there aren't any earlier
// the first (nonce=0) message onw the context, for one of the members, and there aren't any earlier
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

// We must block here long enough to get the payload from the publicstorage, persist the messages in the correct
// sequence, and also persist all the data.
func (em *eventManager) BatchPinComplete(bi blockchain.Plugin, batchPin *blockchain.BatchPin, signingIdentity string, protocolTxID string, additionalInfo fftypes.JSONObject) error {
func (em *eventManager) BatchPinComplete(bi blockchain.Plugin, batchPin *blockchain.BatchPin, author string, protocolTxID string, additionalInfo fftypes.JSONObject) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency in terminology, isn't this what we're calling a "key" (ie an Ethereum address)? Seems like the word "author" has evolved to mean a higher level FireFly/DID string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was transposed to signingIdentity below anyway, so using that consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think there might have been some earlier confusion that the pins table should contain resolved identities, whereas it is intentional that pins are just references to a pin received and validated that it matched the signing identity in the transaction object.

}

// THIS IS WHERE THE OLD CODE FIRST CALLS ResolveInputIdentity
// It does NOT have an author and key the first time
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment feels like it can be removed. Don't think there's any need to reference OLD CODE vs NEW CODE, but we should note if there's a specific contract coming into a method (ie the identity must/must not be resolved when calling this).


orgKey := config.GetString(config.OrgKey)
if orgKey == "" {
orgKey = config.GetString(config.OrgIdentityDeprecated)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a helper for getting the org key, which checks both the new and deprecated key? Seems like this pattern is repeated in a few places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

helper, mock, and test updates added

return node, nil
}

func (pm *privateMessaging) getReceipients(ctx context.Context, in *fftypes.MessageInOut) (gi *fftypes.GroupIdentity, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not your change, but there's a typo in this method name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

if err != nil {
l.Errorf("Invalid batch '%s'. Author '%s' cound not be resolved: %s", batch.ID, batch.Author, err)
return false, nil // This is not retryable. skip this batch
return false, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this become retryable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - will resolve this

return valid, err
}

func (em *eventManager) isRootOrgBroadcast(batch *fftypes.Batch) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels...weird. Should we be tagging these messages in some special way? Unpacking all the data of every message in every batch to look for this special situation just feels very brute-force.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - think there's an issue in the code, where it should be doing this only in the case where we get an error verifying the identity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - that's resolved now

node.Name = config.GetString(config.OrgIdentity)
orgName := config.GetString(config.OrgName)
if orgName != "" {
node.Name = fmt.Sprintf("%s.node", config.GetString(config.OrgName))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can reuse orgName here instead of fetching again from config.

Copy link
Contributor

@awrichar awrichar left a comment

Choose a reason for hiding this comment

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

I left a lot of comments, but they're mostly in the category of cleanup rather than functional changes.

I think the functionality looks good, although I'll confess it's difficult to follow exactly what the new rules are for when identities are resolved in each possible flow (some documentation to lay those down would be helpful). I'm relying largely on the E2E tests to assert that no major flows have been broken here =)

Copy link
Contributor Author

@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.

Submitted PR for coverage and code duplication, continuing review.

log.L(ctx).Errorf("Invalid signing identity '%s': %s", batch.Author, err)
return err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It comes from the messages in the batch which must each have been resolved, and will be verified by all members processing the batch, so doing an additional pre-submission check here does not seem efficient.

}

func (bm *broadcastManager) BroadcastMessageWithID(ctx context.Context, ns string, id *fftypes.UUID, unresolved *fftypes.MessageInOut, resolved *fftypes.Message, waitConfirm bool) (out *fftypes.Message, err error) {
func (bm *broadcastManager) BroadcastMessageWithResolvedID(ctx context.Context, ns string, in *fftypes.MessageInOut, waitConfirm bool) (out *fftypes.Message, err error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, pulling it out. Think this became unnecessary in some of the updates @nguyer made

resolved.Header.TxType = fftypes.TransactionTypeBatchPin
}

if !identityResolved {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think this is because the BroadcastRootOrgDefinition method was separated out, which is missing coverage.
Addressing this.

Copy link
Contributor Author

@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.

@awrichar - believe I've addressed all the comments now.

// We must block here long enough to get the payload from the publicstorage, persist the messages in the correct
// sequence, and also persist all the data.
func (em *eventManager) BatchPinComplete(bi blockchain.Plugin, batchPin *blockchain.BatchPin, signingIdentity string, protocolTxID string, additionalInfo fftypes.JSONObject) error {
func (em *eventManager) BatchPinComplete(bi blockchain.Plugin, batchPin *blockchain.BatchPin, author string, protocolTxID string, additionalInfo fftypes.JSONObject) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was transposed to signingIdentity below anyway, so using that consistently.

// We must block here long enough to get the payload from the publicstorage, persist the messages in the correct
// sequence, and also persist all the data.
func (em *eventManager) BatchPinComplete(bi blockchain.Plugin, batchPin *blockchain.BatchPin, signingIdentity string, protocolTxID string, additionalInfo fftypes.JSONObject) error {
func (em *eventManager) BatchPinComplete(bi blockchain.Plugin, batchPin *blockchain.BatchPin, author string, protocolTxID string, additionalInfo fftypes.JSONObject) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think there might have been some earlier confusion that the pins table should contain resolved identities, whereas it is intentional that pins are just references to a pin received and validated that it matched the signing identity in the transaction object.

if err != nil {
l.Errorf("Invalid batch '%s'. Author '%s' cound not be resolved: %s", batch.ID, batch.Author, err)
return false, nil // This is not retryable. skip this batch
return false, err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - will resolve this

if author != id.OnChain {
l.Errorf("Invalid batch '%s'. Author '%s' does not match transaction submitter '%s'", batch.ID, id.OnChain, author)

isRootOrgBroadcast, err := em.isRootOrgBroadcast(batch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no error return from this function, removing.

return valid, err
}

func (em *eventManager) isRootOrgBroadcast(batch *fftypes.Batch) (bool, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok - that's resolved now


orgKey := config.GetString(config.OrgKey)
if orgKey == "" {
orgKey = config.GetString(config.OrgIdentityDeprecated)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

helper, mock, and test updates added

return node, nil
}

func (pm *privateMessaging) getReceipients(ctx context.Context, in *fftypes.MessageInOut) (gi *fftypes.GroupIdentity, err error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

)

// SystemCore specifies an interface for global utility functions, without creating a cycle between components
type SystemCore interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed 👍

@peterbroadhurst peterbroadhurst marked this pull request as ready for review October 4, 2021 18:20
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
peterbroadhurst and others added 4 commits October 4, 2021 14:49
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
As of now, token operations will not include any identity mapping - overriding
any identities for token operations will require passing a string that is understood
by the underlying connector (ie an Ethereum address). For convenience, this
defaults to the identity of the local org.

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

codecov-commenter commented Oct 5, 2021

Codecov Report

Merging #192 (0880ed5) into main (f435cbf) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 0880ed5 differs from pull request most recent head 6f62a4a. Consider uploading reports for the commit 6f62a4a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##              main      #192    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          206       207     +1     
  Lines        11163     11318   +155     
==========================================
+ Hits         11163     11318   +155     
Impacted Files Coverage Δ
pkg/fftypes/batch.go 100.00% <ø> (ø)
pkg/fftypes/message.go 100.00% <ø> (ø)
pkg/fftypes/tokenpool.go 100.00% <ø> (ø)
pkg/wsclient/wstestserver.go 100.00% <ø> (ø)
internal/assets/manager.go 100.00% <100.00%> (ø)
internal/assets/token_pool_created.go 100.00% <100.00%> (ø)
internal/batch/batch_manager.go 100.00% <100.00%> (ø)
internal/batch/batch_processor.go 100.00% <100.00%> (ø)
internal/batchpin/batchpin.go 100.00% <100.00%> (ø)
internal/blockchain/ethereum/config.go 100.00% <100.00%> (ø)
... and 40 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 f435cbf...6f62a4a. Read the comment docs.

peterbroadhurst and others added 3 commits October 6, 2021 15:04
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
type Identity struct {
Identifier string `json:"identifier,omitempty"`
OnChain string `json:"onchain,omitempty"`
Author string `json:"author,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

feels like Identifier would be a better name here, since the identity is used in more scenarios than sending/broadcasting messages, such as an org's Identity that's solely used to endorse other identities

Copy link
Contributor

Choose a reason for hiding this comment

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

never mind this one, was from an earlier thought (days ago) that has since been discarded, but neglected to delete the comment

BEGIN;

ALTER TABLE batches ADD "key" VARCHAR(1024);
UPDATE batches SET "key" = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

the empty string should be delimited by single quotes instead of double quotes, otherwise you'd get the following error during startup:

FF10163: Database migration failed: FF10163: Database migration failed: migration failed: zero-length delimited identifier at or near """" (column 28) in line 4: BEGIN;

ALTER TABLE batches ADD "key" VARCHAR(1024);
UPDATE batches SET "key" = "";
ALTER TABLE batches ALTER COLUMN "key" SET NOT NULL;

ALTER TABLE messages ADD "key" VARCHAR(1024);
UPDATE messages SET "key" = "";
ALTER TABLE messages ALTER COLUMN "key" SET NOT NULL;

ALTER TABLE tokenpool ADD "key" VARCHAR(1024);
UPDATE tokenpool SET "key" = "";
ALTER TABLE tokenpool ALTER COLUMN "key" SET NOT NULL;

@@ -0,0 +1,8 @@
ALTER TABLE batches ADD COLUMN "key" VARCHAR(1024);
UPDATE batches SET "key" = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, single quotes instead of double quotes

Signed-off-by: Jim Zhang <jim.zhang@kaleido.io>
@jimthematrix jimthematrix merged commit 06dedc5 into hyperledger:main Oct 6, 2021
@jimthematrix jimthematrix deleted the identity branch October 6, 2021 21:55
@jimthematrix jimthematrix mentioned this pull request Oct 8, 2021
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.

5 participants