Fix: Solve issue #331 by adding FirstEvent support in FireFly config#332
Open
MKVEERENDRA wants to merge 3 commits intohyperledger:mainhyperledger/firefly-cli:mainfrom
Open
Fix: Solve issue #331 by adding FirstEvent support in FireFly config#332MKVEERENDRA wants to merge 3 commits intohyperledger:mainhyperledger/firefly-cli:mainfrom
MKVEERENDRA wants to merge 3 commits intohyperledger:mainhyperledger/firefly-cli:mainfrom
Conversation
Author
|
I Fixed it |
…Fly config Signed-off-by: MKVEERENDRA <veerendravamsi66@gmail.com>
99368a5 to
151290f
Compare
EnriqueL8
requested changes
Nov 19, 2024
cmd/start.go
Outdated
|
|
||
| func init() { | ||
| startCmd.Flags().BoolVarP(&startOptions.NoRollback, "no-rollback", "b", false, "Do not automatically rollback changes if first time setup fails") | ||
| startCmd.Flags().StringVarP(&startOptions.FirstEvent, "first-event", "f", "0", "Specify the starting block for event processing (default: 0)") |
Contributor
There was a problem hiding this comment.
A few things:
- Move this to the init command
- Would suggest renaming to
--from-blockand I wouldn't put a shortcut for this one of-f - I would make this super obvious that is only applies to multiparty as a contract listener gets created for that contract... It might be better as well to not create the contract listener at all
Suggested change
| startCmd.Flags().StringVarP(&startOptions.FirstEvent, "first-event", "f", "0", "Specify the starting block for event processing (default: 0)") | |
| startCmd.Flags().StringVarP(&startOptions.FirstEvent, "--from-block", "f", "0", "Specify the starting block for event processing (default: 0)") |
internal/stacks/stack_manager.go
Outdated
| if err := s.patchFireFlyCoreConfigs(configDir, member, newConfig); err != nil { | ||
| return messages, err | ||
| } | ||
| if s.Stack.MultipartyEnabled { |
Contributor
There was a problem hiding this comment.
and not sure why code was removed to write to the state file her
| for _, member := range s.Stack.Members { | ||
| orgConfig := s.blockchainProvider.GetOrgConfig(s.Stack, member) | ||
| newConfig.Namespaces.Predefined[0].DefaultKey = orgConfig.Key | ||
| if s.Stack.MultipartyEnabled { |
Contributor
There was a problem hiding this comment.
we still need this MultipartyEnabled wrapping this section
|
|
||
| for _, member := range s.Stack.Members { | ||
| orgConfig := s.blockchainProvider.GetOrgConfig(s.Stack, member) | ||
| newConfig.Namespaces.Predefined[0].DefaultKey = orgConfig.Key |
Signed-off-by: MKVEERENDRA <veerendravamsi66@gmail.com>
Signed-off-by: MKVEERENDRA <136776337+MKVEERENDRA@users.noreply.github.com>
Author
|
I did what you said can you pls check out the commit |
EnriqueL8
requested changes
Nov 27, 2024
Contributor
EnriqueL8
left a comment
There was a problem hiding this comment.
Quite a few comments, and see many problems with the structure of the code and the types. Please do add some tests and try this out. Keep the comments that were previously put in by other contributors when the logic has not changed.
| When initializing a new stack, you can configure various options including: | ||
|
|
||
| ``` | ||
| --from-block For multiparty networks only: specify the starting block for event processing. |
Contributor
There was a problem hiding this comment.
Should only work with remote-node-url as with the local chain that the CLI creates we want to start from 0
Contributor
There was a problem hiding this comment.
as it should point to when the contract was deployed - I'm more tempted for this to be --contract-block as you pass in the MultiParty Contract address through --contract
Comment on lines
+70
to
+77
| # Initialize with default settings (starts from block 0) | ||
| ff init mystack | ||
|
|
||
| # Initialize starting from the latest block | ||
| ff init mystack --from-block newest | ||
|
|
||
| # Initialize starting from a specific block number | ||
| ff init mystack --from-block 1234567 |
Contributor
There was a problem hiding this comment.
I would make these examples to be with --emote-node-url set
| } | ||
|
|
||
|
|
||
| // ... existing code ... |
| fmt.Printf("starting FireFly stack '%s'... ", s.Stack.Name) | ||
| // Check to make sure all of our ports are available | ||
|
|
||
| // Check port availability |
|
|
||
| //nolint:gocyclo // TODO: Breaking this function apart would be great for code tidiness, but it's not an urgent priority | ||
| func (s *StackManager) runFirstTimeSetup(options *types.StartOptions) (messages []string, err error) { | ||
| func (s *StackManager) runFirstTimeSetup(options *types.InitOptions) (messages []string, err error) { |
Contributor
There was a problem hiding this comment.
This should still be the StartOptions
Comment on lines
-946
to
-964
| // TODO: This code assumes that there is only one plugin instance per type. When we add support for | ||
| // multiple namespaces, this code will likely have to change a lot | ||
| s.Log.Info("deploying FireFly smart contracts") | ||
| contractDeploymentResult, err = s.blockchainProvider.DeployFireFlyContract() | ||
| if err != nil { | ||
| return messages, err | ||
| } | ||
| if contractDeploymentResult != nil { | ||
| if contractDeploymentResult.Message != "" { | ||
| messages = append(messages, contractDeploymentResult.Message) | ||
| } | ||
| s.Stack.State.DeployedContracts = append(s.Stack.State.DeployedContracts, contractDeploymentResult.DeployedContract) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for _, member := range s.Stack.Members { | ||
| orgConfig := s.blockchainProvider.GetOrgConfig(s.Stack, member) | ||
| newConfig.Namespaces.Predefined[0].DefaultKey = orgConfig.Key |
Contributor
There was a problem hiding this comment.
As I mentioned in my previous review, keep this code as it was
Comment on lines
+626
to
+628
| setupMessages, err := s.runFirstTimeSetup(&types.InitOptions{ | ||
| FromBlock: options.FromBlock, | ||
| }) |
Contributor
There was a problem hiding this comment.
FromBlock doesn't exist in InitOptions, it should be StartOptions. Getting errors in the IDE
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses issue #331, which was about adding support for the FirstEvent parameter in the FireFly configuration files.
Changes Made:
Added a new FirstEvent field support in the FireFly configuration (firefly_core_*.yml) files to allow specifying the starting event for blockchain-based processes.
Integrated this new parameter into the FireFly configuration logic during the stack initialization process.
Updated the stack setup and blockchain deployment flow to support FirstEvent, ensuring it can be configured when setting up FireFly for the first time.
The FirstEvent field is now configurable in the FireFlyConfig and is passed to the blockchain and smart contract initialization process.
Context and Fix:
The addition of the FirstEvent parameter gives users flexibility in defining which blockchain event the FireFly stack should use when starting up. This was missing in the previous configuration, causing issues for users who needed specific event handling.
This update ensures that FirstEvent is properly handled, stored in the configuration, and used during smart contract and blockchain node setup.
How to Test:
Start the FireFly stack with the new configuration changes.
Verify that the FirstEvent field is correctly passed to the blockchain during FireFly stack initialization.
Ensure that the expected event triggers the FireFly workflow appropriately, depending on the FirstEvent configuration value.
Issue Comment (on issue #331):
I have solved issue #331 by adding support for the FirstEvent parameter in the FireFly configuration. This allows users to specify which event should be used during FireFly stack initialization, enabling more flexible event handling for blockchain-based processes.
The fix is now available in the pull request https://github.com/MKVEERENDRA/firefly-cli1, and it has been tested to ensure the correct configuration of the FirstEvent value during the FireFly setup.