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

Add off-chain-data go client application #1269

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

twoGiants
Copy link
Contributor

@twoGiants twoGiants commented Nov 11, 2024

Off Data Go Application

This PR contains an implementation of the off-chain data store sample in go using the block event listening capability of the Fabric Gateway client API. The issue reference is here in the Fabric Gateway SDK repository.

It follows the same procedure and uses mostly the same naming as the off-chain data typescript sample here. There are some differences in the go package structure compared to the typescript modules and I added a couple of tests to the parser which helped me better understand the unpacking of the block data structure. The Go implementation is more similar to the Java version. More about that below in the Differences section.

@bestbeforetoday gave me this useful link where I found the iterator_test.go to be helpful and this link where the "Endorser Transaction" comments were helpful.

Summary

  • created project directory, app.go, getAllAssets.go, transact.go and listen.go files
  • reused and changed the logic for connect.go from the private data application
  • implemented the asset transfer basic contract in the contract package
  • implemented getAllAssets.go and transact.go
  • implemented the block parser and all the supporting data structures
  • implemented the block processor and all the supporting data structures
  • implemented listen.go
  • updated README.md with the command to run the go application
  • updated script which runs the application in the Github Actions workflow
  • manual tests

Differences

  • I didn't use any interfaces. I removed them after the suggestion of @bestbeforetoday.
  • I did not implement any methods which are not used anywhere, like toProto or getSignatureHeader.
  • The block parser with all it's underlying structs (which are mostly private) is in the parser package.
  • There are some simple tests for the cache utility and the block unwrapping.
  • The block processor with it's underlying struct transaction (which is private) is in the processor package.
  • The store is in the store package and because it resembles a bit a flat file db I called it flatFile.go
  • The utilities are in the utils package.
  • In all the additional packages I followed the encapsulation rule: if it's not referenced outside the package it's private.

Additional Information

I process the block events in a go routine and use context to stop the processing. Instead of panicking everywhere down the line where an error can occur I handle the errors by wrapping them, returning them, log them in the go routine, then return and shutdown gracefully. Panicking results in the context, gateway and checkpointer not getting closed.

Questions

  1. Here is a TODO proposing to simplify the processor.transaction data structure.
  2. And here is a TODO proposing to encapsulate the final unwrapping of the kvWrite in the parser instead of having it in transaction.writes.
  3. I assume I should squash everything to one commit? It might be a big commit message.

@twoGiants twoGiants force-pushed the off-chain-data-go branch 3 times, most recently from 45a2aac to 09402bc Compare November 15, 2024 18:02
@twoGiants twoGiants force-pushed the off-chain-data-go branch 3 times, most recently from f7ede20 to 4e58c31 Compare November 25, 2024 14:56
@twoGiants twoGiants force-pushed the off-chain-data-go branch 3 times, most recently from 307bb47 to 43f67fb Compare January 4, 2025 11:04
@twoGiants twoGiants force-pushed the off-chain-data-go branch 2 times, most recently from 433bd6a to 77e19a4 Compare January 7, 2025 10:09
Created project structure, fixed typos. Implemented connect.go and
getAllAssets.go. The latter uses an assetTransferBasic struct which
provides a simple API for basic asset operations like create, transfer,
etc.

Added transact.go with some util functions. Using google uuid package to
generate random UUIDs for the transactions.

Implemented pretty printing of JSON results.

Implemented app.go entry point with error handling. The existing
commands are getAllAssets, transact and listen. They can be called from
the command line via: "go run . <command> <command> ...". They will be
executed in order and if a command is not known an the application
panics and aborts before executing any of the commands.

Implementing listen.go. Added checkpointer, context setups, call to
BlockEvents and all the interfaces needed for parsing. Started
implementing the interfaces needed to represent a block bottom up in
structs. Finished NamespaceReadWriteSet, ReadWriteSet and
EndorserTransaction.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Signed-off-by: Stanislav Jakuschevskij <[email protected]>
For the GetCreator() method return the identity.Identity interface was
also implemented.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Created parser, contract and utils packages and extracted each piece of
functionality into its own files. Removed "Get" prefix from methods and
changed return values from interfaces to structs.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Removed Block and Transaction interfaces and unused statusCode function.
Using the struct instead of the interfaces now.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Added block processor struct and the process method.
Implemented getting valid transactions from the last processed index.
Added data structures needed for the store.

Decomposed the parser.Block.Transactions() method into readable chunks.

Added transaction processor struct and process method. Unwrapping
read write set data from the transaction, mapping to a new "write"
data structure and passing down to the store.

Store is an empty function and will be implemented next.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Persisting ledger writes to the file system into the store.log file in
the application-go directory. The write values are converted from bytes
to a string when the read write sets are unwrapped in the transaction
processor.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Added caching util function with tests and applied in:

- parser.Block.Transactions(),
- parser.Payload.ChannelHeader(),
- parser.Payload.SignatureHeader(),
- parser.NamespaceReadWriteSet.ReadWriteSet(),
- parser.EndorserTransaction.ReadWriteSets(),

methods, as it was in the typescript sample.

Corrected Println usage and added comments to util functions.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Created packages for the flat file store and the processor and moved
functions, variables and constants from listener.go to those packages.
Encapsulated everything not used outside the packages, introduced
model.go files which later might be extracted into a model package and
renamed parser/parsedBlock.go to parser/block.go.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Before when pressing 'ctrl+c' and stopping the go program non of the
deferred functions in listen.go were called. A standard procedure for
stopping goroutines with context was implemented which shuts down the
program gracefully. Logs were added to notify the user that the shutdown
was successful.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Every struct was put in its own file. Every method which is not used
outside the parser package was given package scope. All interfaces were
removed, they are implemented by the structs which are now used
everywhere needed as return values. There is no clear benefit of using
interfaces in this sample.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Before all transactions were processed and when the failure was
simulated a message was printed and all the transactions still
processed. Now the store returns an error when the failure is simulated
which the listener expects so that it can gracefully shutdown the system
and close the context. The context must be closed correctly or the
checkpointer won't save the last processed transactionId to the file
system.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
@twoGiants twoGiants force-pushed the off-chain-data-go branch 2 times, most recently from 9364108 to 0b8ba91 Compare January 8, 2025 15:32
Starting from the processor.Block.Process all methods now return errors
if something goes wrong with unpacking of the blocks and reading the
transactions. In each function where the error is being propagated back
to client it is wrapped in a message with the function name. This makes
it easier to track down the error and see the propagation chain. Finally
the error is logged to the terminal and the go routine shuts down
gracefully. The graceful shutdown executes all deferred functions which
close the context, the checkpointer and the gateway.

Before panics were used everywhere which was an issue because the
unpacking of the blocks happened in a go routine. When a panic happens
in a go routine only the deferred functions of the go routine are called
but not those of the client which lead to unexpected behavior.

The transact function is also executed in a go routine therefore the
same typo of error handling was implemented there.

Signed-off-by: Stanislav Jakuschevskij <[email protected]>
Signed-off-by: Stanislav Jakuschevskij <[email protected]>
@twoGiants twoGiants changed the title 🚧 WIP: Add off-chain-data go client application 🚧 Add off-chain-data go client application Jan 9, 2025
@twoGiants twoGiants marked this pull request as ready for review January 9, 2025 10:32
@twoGiants twoGiants requested a review from a team as a code owner January 9, 2025 10:32
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

First thing, it runs great for me so this looks nicely done. Thank you! Sorry for large number of comments. This is just polishing the material to make it a really clean sample.

I would be careful to try and keep all the code that listens for blocks, navigates to their transactions and passes the read/write sets to some kind of store close together. Perhaps all in listen.go. This is the key learning point of the sample so it needs to be easy to access and navigate. The block parser should be separate (as you've done) so it is a reusable package on its own. The store implementation can also be a little saperate to avoid cluttering the key listener code. I might put it in a different file within the same package though, just as private (lowercase) content.

The Application section of off_chain_data/README.md should be updated to include references to key parts of the Go implementation, similar to the existing TypeScript and Java entries.

To catch some issues, it might be worth running this command in the application-go directory:

go run honnef.co/go/tools/cmd/staticcheck@latest -f stylish -checks all ./...

To avoid specific checks (like having package-level godoc), you can exclude specific rules. For example:

go run honnef.co/go/tools/cmd/staticcheck@latest -f stylish -checks 'all,-ST1000' ./...

"google.golang.org/grpc"
)

var allCommands = map[string]func(clientConnection *grpc.ClientConn){
Copy link
Member

Choose a reason for hiding this comment

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

The clientConnection parameter name can be omitted here. Your choice which you find clearer though.

Suggested change
var allCommands = map[string]func(clientConnection *grpc.ClientConn){
var allCommands = map[string]func(*grpc.ClientConn){

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Omitted.

return fmt.Errorf("in %s: %w", funcName, err)
}
transactionId := channelHeader.GetTxId()
b.checkpointer.CheckpointTransaction(blockNumber, transactionId)
Copy link
Member

Choose a reason for hiding this comment

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

The returned error value should be checked to ensure the write was successful.

Suggested change
b.checkpointer.CheckpointTransaction(blockNumber, transactionId)
if err := b.checkpointer.CheckpointTransaction(blockNumber, transactionId); err != nil {
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.

Thx. Done.

b.checkpointer.CheckpointTransaction(blockNumber, transactionId)
}

b.checkpointer.CheckpointBlock(blockNumber)
Copy link
Member

Choose a reason for hiding this comment

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

The returned error value should be checked to ensure the write was successful.

Suggested change
b.checkpointer.CheckpointBlock(blockNumber)
return b.checkpointer.CheckpointBlock(blockNumber)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx. Done.

Comment on lines +14 to +22
var (
colors = []string{"red", "green", "blue"}
Owners = []string{"alice", "bob", "charlie"}
)

const (
maxInitialValue = 1000
maxInitialSize = 10
)
Copy link
Member

Choose a reason for hiding this comment

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

This is only used by the transact command logic. It is not really relevant to the asset type definition. I would move it to the transact command implementation.

Comment on lines +32 to +45
func NewAsset() Asset {
id, err := uuid.NewRandom()
if err != nil {
panic(err)
}

return Asset{
ID: id.String(),
Color: utils.RandomElement(colors),
Size: uint64(utils.RandomInt(maxInitialSize) + 1),
Owner: utils.RandomElement(Owners),
AppraisedValue: uint64(utils.RandomInt(maxInitialValue) + 1),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is only used by the transact command logic. It is not really relevant to the asset type definition. I would move it to the transact command implementation.

Copy link
Contributor Author

@twoGiants twoGiants Jan 10, 2025

Choose a reason for hiding this comment

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

I am also using it in the parser/block_test.go. I think keeping the Asset construction next to the asset is cohesive or wdys? But being that it's only used in transact I can see your point also.

I will move it to transact, it's no problem and use a simpler construction in the test.

Comment on lines +19 to +32
return fmt.Sprintf(format, t.txId, t.blockNumber, t.blockTxIdsJoinedByComma())
}

func (t *txIdNotFoundError) blockTxIdsJoinedByComma() string {
result := ""
for index, item := range t.blockTxIds {
if len(t.blockTxIds)-1 == index {
result += item
} else {
result += item + ", "
}
}
return result
}
Copy link
Member

Choose a reason for hiding this comment

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

The standard library has a function to join an array of strings. Using this simplifies this type enough that I would consider removing it entirely and just building a generic error using fmt.Errorf at the point of use.

Suggested change
return fmt.Sprintf(format, t.txId, t.blockNumber, t.blockTxIdsJoinedByComma())
}
func (t *txIdNotFoundError) blockTxIdsJoinedByComma() string {
result := ""
for index, item := range t.blockTxIds {
if len(t.blockTxIds)-1 == index {
result += item
} else {
result += item + ", "
}
}
return result
}
return fmt.Sprintf(format, t.txId, t.blockNumber, strings.Join(t.blockTxIds, ", "))
}

func (atb *AssetTransferBasic) GetAllAssets() ([]byte, error) {
result, err := atb.contract.Evaluate("GetAllAssets")
if err != nil {
return []byte{}, fmt.Errorf("in GetAllAssets: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

See https://go.dev/wiki/CodeReviewComments#declaring-empty-slices

Suggested change
return []byte{}, fmt.Errorf("in GetAllAssets: %w", err)
return nil, fmt.Errorf("in GetAllAssets: %w", err)


type Block struct {
block *common.Block
transactions []*Transaction
Copy link
Member

Choose a reason for hiding this comment

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

transactions is never used

Comment on lines +65 to +91
var wg sync.WaitGroup
wg.Add(1)

go func() {
defer wg.Done()

for blockProto := range blocks {
select {
case <-ctx.Done():
return
default:
blockProcessor := processor.NewBlock(
parser.ParseBlock(blockProto),
checkpointer,
store.ApplyWritesToOffChainStore,
channelName,
)

if err := blockProcessor.Process(); err != nil {
fmt.Println("\033[31m[ERROR]\033[0m", err)
return
}
}
}
}()

wg.Wait()
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure of the reason for using a wait group for a single goroutine that you then wait on before continuing. Maybe this can just be inline code?

It should not be necessary to check the context while reading. If the context used for the BlockEvents call is closed, the returned channel should also get closed, which would mean the range loop will exit.

Suggested change
var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
for blockProto := range blocks {
select {
case <-ctx.Done():
return
default:
blockProcessor := processor.NewBlock(
parser.ParseBlock(blockProto),
checkpointer,
store.ApplyWritesToOffChainStore,
channelName,
)
if err := blockProcessor.Process(); err != nil {
fmt.Println("\033[31m[ERROR]\033[0m", err)
return
}
}
}
}()
wg.Wait()
for blockProto := range blocks {
blockProcessor := processor.NewBlock(
parser.ParseBlock(blockProto),
checkpointer,
store.ApplyWritesToOffChainStore,
channelName,
)
if err := blockProcessor.Process(); err != nil {
fmt.Println("\033[31m[ERROR]\033[0m", err)
return
}
}

@@ -0,0 +1,20 @@
module offChainData
Copy link
Member

Choose a reason for hiding this comment

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

By convention Go package names are all lowercase. See https://go.dev/blog/package-names

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.

2 participants