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

Retry tokens events consistent with FFDX, as we cannot push back nack #1296

Merged
merged 3 commits into from
May 3, 2023

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented May 3, 2023

Make this consistent with what we did for FFDX, and prevents a scenario on startup as seen in the integration test where a token event comes in before the namespace is started:

firefly_core_0_1  | [2023-04-30T00:35:08.431Z] DEBUG Calling TokenPoolCreated callback. Locator='address=0x8875e026c26cedc2ab9880ac56d31e328428db73&schema=ERC20WithData&type=fungible' TX=token_pool/66fffabe-5b11-4118-abc2-50b95877654d pid=1 proto=fftokens role=event-loop
firefly_core_0_1  | [2023-04-30T00:35:08.431Z] ERROR Event loop exiting (FF10446: Namespace 'default' is not started). Terminating server! pid=1 proto=fftokens role=event-loop

Also made a change to both FFDX and FFTokens to split eventRetry out from retry in the config, as I realized looking at this PR that the config was overlapping with the ffresty config for request retry.

… to tokens connector

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

codecov-commenter commented May 3, 2023

Codecov Report

Merging #1296 (e4218ea) into main (2c30d54) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1296   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          308       308           
  Lines        20678     20692   +14     
=========================================
+ Hits         20678     20692   +14     
Impacted Files Coverage Δ
internal/dataexchange/ffdx/config.go 100.00% <100.00%> (ø)
internal/dataexchange/ffdx/ffdx.go 100.00% <100.00%> (ø)
internal/tokens/fftokens/config.go 100.00% <100.00%> (ø)
internal/tokens/fftokens/fftokens.go 100.00% <100.00%> (ø)

Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
@@ -346,12 +354,15 @@ func (ft *FFTokens) handleTokenPoolCreate(ctx context.Context, data fftypes.JSON
blockchainEvent := data.GetObject("blockchain")

poolDataString := data.GetString("data")
if poolData == nil && poolDataString != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In unit testing, I found the poolData == nil && poolDataString == "" case was uncaught, and resulted in a panic

Copy link
Contributor

Choose a reason for hiding this comment

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

#1121? 🙏

@peterbroadhurst
Copy link
Contributor Author

Diagnosing whether this test failure is related to the code change

{"name":"test_1683116741440968188","members":[{"identity":"org_9571df"},{"identity":"org_11bbf0"}]}}
    restclient.go:116: 2023-05-03T12:26:00.712068565Z: <== 500
    restclient.go:118: <!! {"error":"FF10277: Identity could not be resolved via lookup string 'org_11bbf0'"}
    restclient.go:119: Headers: map[Content-Length:[83] Content-Type:[application/json] Date:[Wed, 03 May 2023 12:26:00 GMT] Vary:[Origin]]
    restclient.go:499: Sent private message  to [{Identity:org_9571df Node:} {Identity:org_11bbf0 Node:}]
    contract_migration.go:180: 
        	Error Trace:	/home/runner/work/firefly/firefly/test/e2e/runners/contract_migration.go:180
        	            				/home/runner/work/firefly/firefly/test/e2e/runners/contract_migration.go:51
        	Error:      	Not equal: 
        	            	expected: 202
        	            	actual  : 500
        	Test:       	TestEthereumMultipartyE2ESuite/TestContractMigration

@peterbroadhurst
Copy link
Contributor Author

#1296 (comment) appears unrelated to the change, but does need diagnosis. Raised #1297

@peterbroadhurst
Copy link
Contributor Author

peterbroadhurst commented May 3, 2023

Fixes #1121

@peterbroadhurst peterbroadhurst merged commit 0350d38 into main May 3, 2023
@peterbroadhurst peterbroadhurst deleted the fft-retry branch May 3, 2023 18:26
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