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.

Sign up for GitHub

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

Jump to bottom

[EventsReplayClient] Fix Replay Client Bugs #267

Merged
merged 34 commits into from
Dec 23, 2023
Merged

[EventsReplayClient] Fix Replay Client Bugs #267

merged 34 commits into from
Dec 23, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Dec 12, 2023 •
edited by reviewpad bot
Loading

Summary

Human Summary

This PR implements remapping of events to allow the usage of EventsSequence (CommittedBlocksSequence and RedelegationsSequence) to be maintained between the EventsQueryClient closing.

It also fixes numerous bugs where the EventsReplayClient was not behaving as expected:

  • retry.OnError logic has been fixed
  • The EventsQueryClient closing causing the above to occur has been added
  • Logic for remapping events from one observable to another has been improved

These are all covered in the regression integration test added which now acts as a backstop protecting these features from breaking.

AI Summary

Summary generated by Reviewpad on 22 Dec 23 16:23 UTC

This pull request includes the following changes:

  • Added a new error variable ErrEventsConsClosed in pkg/client/events/errors.go.
  • Changes to the file client.go indicate the removal of cometWebsocketURL from the NewDelegationClient() function and its related calls.
  • Changes in query_client.go include modifications to code blocks and locking/unlocking calls.
  • Modifications in pkg/client/block/client_integration_test.go involve the removal of build tags and the addition of test functions and import statements.
  • Changes in connection.go involve imports, addition of new functions, and modifications related to managing mock objects for testing.
  • Modifications in the "openapi.yml" file include updates to titles and options for the RPC type.
  • Changes in cmd.go indicate the removal of arguments from function calls in NewSupplyBlockClientFn() and NewSupplyPOKTRollSDKFn().
  • Modifications in a file indicate the removal of a method and changes to other methods in terms of their functionality and comments.
  • Changes in the file client.go involve variable declaration and function call modifications.
  • The diff in the file client.go shows changes related to import statements and function arguments.
  • Modifications in the file client.go involve the removal of import statements and changes in variable initialization.
  • Changes in the file client.go involve the removal of a function call argument.
  • A new integration test file replay_client_integration_test.go has been added with a test function.
  • Changes in replay.go involve synchronization, error messages, and comment updates related to context cancellation and unsubscribe functionality.
  • Changes in a file include import statements, variable assignments, and function call modifications.
  • The file client_integration_test.go has experienced syntactical modifications, changes in comments, and modifications in test functions and assertions.
  • The diff in the file deps_builder.go shows the removal of a parameter in a function call.
  • Changes in the file godoc.go involve updating package documentation comments with a focus on redelegation events.
  • Changes in the file replay_client.go involve retrying the events query, modification of the replay observable buffer size, and changes to function signatures.
  • The diff in the file client.go shows the removal of a function call argument.
  • Changes in the file involve logging error messages during retries with improved logging output.
  • The file client_test.go underwent changes related to import statements and function call modifications.
  • The diff in the file includes the removal of an import statement and modification of a function call argument.
  • The summary of the file diff includes the removal of an import statement and modification of a function call argument.
  • Changes in a file involve modifying a function call by removing an argument.

Please let me know if you need more information or if you have any other questions regarding this pull request.

Issue

  • N/A

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_develop_and_test
  • Run E2E tests locally: make test_e2e
  • **Run E2E tests on DevNet: Add the devnet-test-e2e` label to the PR. This is VERY expensive, o only do it after all the reviews are complete.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@h5law h5law added enhancement New feature or request off-chain Off-chain business logic labels Dec 12, 2023
@h5law h5law added this to the Shannon TestNet milestone Dec 12, 2023
@h5law h5law self-assigned this Dec 12, 2023
bryanchriswhite
bryanchriswhite requested changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the assist! 😎

I left a few comments regarding how/what we should be testing but otherwise, this is a solid improvement and bug fix. 🦾

@h5law
Squashed commit of the following:
7e19624 
commit d621631
Author: Redouane Lakrache <[email protected]>
Date:   Wed Dec 13 16:24:34 2023 +0100

    [SDK] feat: Have distinct JSON-RPC and gRPC urls (#261)

    * feat: Have distinct JSON-RPC and gRPC urls

    * chore: Trigger e2e tests

    * chore: Fix import groups

commit 4e30e27
Author: Bryan White <[email protected]>
Date:   Wed Dec 13 14:28:36 2023 +0100

    fix PR template testing checklist item (#268)
@h5law
chore: remove unneeded ws url
67e1284
@h5law
chore: remove unneeded mock creator
a2d0f6e
@h5law
feat: add initial remapping integration test
cc639b9
@h5law
chore: skip test for now
edebb89
@h5law
squash: merge with main
79dd438 
commit df73cfa
Author: Bryan White <[email protected]>
Date:   Thu Dec 14 01:06:03 2023 +0100

    refactor: `NewMinedRelay` to shared testutil (#262)

commit 410965a
Author: Bryan White <[email protected]>
Date:   Thu Dec 14 01:05:52 2023 +0100

    fix: PR template typo 2 (#269)

commit dd7df68
Author: Daniel Olshansky <[email protected]>
Date:   Wed Dec 13 13:45:26 2023 -0800

    [Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" (#253)

    This is aiming to fix multiple issues in E2E tests and just general QoL improvements:

    1. Adding a comment in `relay.feature` about the need to run `make supplier1_stake && make app1_stake` until we fix #180 (known issue)
    2. Replacing the reference to host `sequencer-poktroll-sequencer` with `localhost` (see first screenshot below)
    3. Fixed a bug leading us to assert against a `MsgSubmitProof` event instead of a `MsgCreateClaim` event (see second screenshot below)
    4. Fixed on-chain logging under `x/*` by replacing `logger\.(Info|Error)\("([^"]+)"(?:, (.*))?\)` with `logger.$1(fmt.Sprintf("$2", $3))` so it works as expected
    5. Some quality-of-life comments & TODOs
    6. Minor additions to unit tests from #220

commit d621631
Author: Redouane Lakrache <[email protected]>
Date:   Wed Dec 13 16:24:34 2023 +0100

    [SDK] feat: Have distinct JSON-RPC and gRPC urls (#261)

    * feat: Have distinct JSON-RPC and gRPC urls

    * chore: Trigger e2e tests

    * chore: Fix import groups

commit 4e30e27
Author: Bryan White <[email protected]>
Date:   Wed Dec 13 14:28:36 2023 +0100

    fix PR template testing checklist item (#268)
@h5law
Merge branch 'main' into feat/replay_client_remapping
ed1a4d8
@h5law
chore: fix merge errors
54d9b4a
@h5law
feat: fix replay.OnError logic for closed connections and update tests
6df24cb
@github-actions github-actions bot added the push-image CI related - pushes images to ghcr.io label Dec 20, 2023
bryanchriswhite
bryanchriswhite approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

Last last comment from me! 🖖 🚢

pkg/client/events/replay_client_integration_test.go Outdated Show resolved Hide resolved
Olshansk
Olshansk approved these changes Dec 20, 2023
View reviewed changes
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@h5law Please see my last NITs w.r.t comments + Bryan's comment, but otherwise let's get it in!

pkg/client/events/replay_client.go Show resolved Hide resolved
pkg/client/events/replay_client.go Outdated Show resolved Hide resolved
pkg/client/events/replay_client_integration_test.go Show resolved Hide resolved
pkg/client/events/replay_client.go Outdated Show resolved Hide resolved
Olshansk
Olshansk approved these changes Dec 21, 2023
View reviewed changes
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@h5law Can you merge with main & rerun e2e tests?

@h5law
Copy link
Contributor Author

h5law commented Dec 22, 2023

@Olshansk @okdas merged with main and still no luck on devnet e2e tests - locally fine tho so....

@okdas
Copy link
Member

okdas commented Dec 22, 2023

merged with main and still no luck on devnet e2e tests - locally fine tho so....

The issue has been resolved, update from Discord:
Screenshot 2023-12-22 at 12 27 06 PM

Olshansk
Olshansk reviewed Dec 23, 2023
View reviewed changes
pkg/client/block/client_integration_test.go
@@ -19,6 +17,7 @@ import (
const blockIntegrationSubTimeout = 5 * time.Second

func TestBlockClient_LastNBlocks(t *testing.T) {
t.Skip("TODO(@h5law): Figure out how to subscribe to events on the simulated localnet")
Copy link
Member

Choose a reason for hiding this comment

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

Nice

Olshansk
Olshansk approved these changes Dec 23, 2023
View reviewed changes
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

🚢

@h5law h5law merged commit 06f8040 into main Dec 23, 2023
10 checks passed
@h5law h5law deleted the feat/replay_client_remapping branch December 24, 2023 12:41
bryanchriswhite added a commit that referenced this pull request Jan 8, 2024
@bryanchriswhite
Merge remote-tracking branch 'pokt/main' into issues/141/refactor/cla…
f3e7866 
…im-proof

* pokt/main:
  [Docs] Load Test #1 - Plan (#286)
  [Bug] Fix observable error logging (#298)
  [docs] Relayminer config documentation (#288)
  [Configs] Add foundation for RelayMiner operation configs. (#284)
  [SMT] Update to use SMT v0.8.2 (#297)
  [EventsReplayClient] Fix Replay Client Bugs (#267)
bryanchriswhite added a commit that referenced this pull request Jan 8, 2024
@bryanchriswhite
Merge branch 'issues/141/refactor/claim-proof' into issues/141/refact…
df9e3c2 
…or/supplier-keys

* issues/141/refactor/claim-proof:
  chore: add TODOs
  [Docs] Load Test #1 - Plan (#286)
  [Bug] Fix observable error logging (#298)
  [docs] Relayminer config documentation (#288)
  [Configs] Add foundation for RelayMiner operation configs. (#284)
  [SMT] Update to use SMT v0.8.2 (#297)
  [EventsReplayClient] Fix Replay Client Bugs (#267)
bryanchriswhite added a commit that referenced this pull request Jan 8, 2024
@bryanchriswhite
Merge branch 'issues/141/refactor/supplier-keys' into issues/141/refa…
8ab13f0 
…ctor/supplier-errors

* issues/141/refactor/supplier-keys:
  chore: add TODOs
  [Docs] Load Test #1 - Plan (#286)
  [Bug] Fix observable error logging (#298)
  [docs] Relayminer config documentation (#288)
  [Configs] Add foundation for RelayMiner operation configs. (#284)
  [SMT] Update to use SMT v0.8.2 (#297)
  [EventsReplayClient] Fix Replay Client Bugs (#267)
bryanchriswhite added a commit that referenced this pull request Jan 8, 2024
@bryanchriswhite
Merge branch 'issues/141/refactor/supplier-errors' into issues/141/pr…
b4ed47c 
…ep/in-memory-network

* issues/141/refactor/supplier-errors:
  chore: add TODOs
  [Docs] Load Test #1 - Plan (#286)
  [Bug] Fix observable error logging (#298)
  [docs] Relayminer config documentation (#288)
  [Configs] Add foundation for RelayMiner operation configs. (#284)
  [SMT] Update to use SMT v0.8.2 (#297)
  [EventsReplayClient] Fix Replay Client Bugs (#267)
bryanchriswhite added a commit that referenced this pull request Jan 8, 2024
@bryanchriswhite
Merge branch 'issues/141/prep/in-memory-network' into issues/141/feat…
191c956 
…/in-memory-network

* issues/141/prep/in-memory-network:
  chore: add #GetConfig()
  chore: rename InMemoryCosmosNetwork to InMemoryNetwork
  chore: update comment
  chore: review feedback improvements
  chore: add TODOs
  [Docs] Load Test #1 - Plan (#286)
  [Bug] Fix observable error logging (#298)
  [docs] Relayminer config documentation (#288)
  [Configs] Add foundation for RelayMiner operation configs. (#284)
  [SMT] Update to use SMT v0.8.2 (#297)
  [EventsReplayClient] Fix Replay Client Bugs (#267)
bryanchriswhite added a commit that referenced this pull request Jan 8, 2024
@bryanchriswhite
Merge branch 'issues/141/feat/in-memory-network' into issues/141/refa…
d1e68fe 
…ctor/in-memory-network

* issues/141/feat/in-memory-network:
  chore: fix comment typo
  chore: post-merge refactor
  chore: fix comment
  chore: add #GetConfig()
  chore: rename InMemoryCosmosNetwork to InMemoryNetwork
  chore: update comment
  chore: review feedback improvements
  chore: add TODOs
  [Docs] Load Test #1 - Plan (#286)
  [Bug] Fix observable error logging (#298)
  [docs] Relayminer config documentation (#288)
  [Configs] Add foundation for RelayMiner operation configs. (#284)
  [SMT] Update to use SMT v0.8.2 (#297)
  [EventsReplayClient] Fix Replay Client Bugs (#267)
bryanchriswhite added a commit that referenced this pull request Jan 8, 2024
@bryanchriswhite
Merge branch 'issues/141/refactor/in-memory-network' into issues/141/…
973635c 
…refactor/proof-store-indices

* issues/141/refactor/in-memory-network:
  chore: post-merge refactor
  chore: review feedback improvements
  chore: fix comment typo
  chore: post-merge refactor
  chore: fix comment
  chore: add #GetConfig()
  chore: rename InMemoryCosmosNetwork to InMemoryNetwork
  chore: update comment
  chore: review feedback improvements
  chore: add TODOs
  [Docs] Load Test #1 - Plan (#286)
  [Bug] Fix observable error logging (#298)
  [docs] Relayminer config documentation (#288)
  [Configs] Add foundation for RelayMiner operation configs. (#284)
  [SMT] Update to use SMT v0.8.2 (#297)
  [EventsReplayClient] Fix Replay Client Bugs (#267)
@bryanchriswhite bryanchriswhite removed push-image CI related - pushes images to ghcr.io devnet-test-e2e labels May 16, 2024
@github-actions github-actions bot removed the devnet label May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryanchriswhite bryanchriswhite bryanchriswhite approved these changes

@github-actions GitHub Actions GitHub Actions left review comments

@Olshansk Olshansk Olshansk approved these changes

Assignees

@h5law h5law

Labels
enhancement New feature or request off-chain Off-chain business logic
Projects
Shannon
Status: ✅ Done
Milestone
Shannon Private TestNet
Development

Successfully merging this pull request may close these issues.
4 participants
@h5law @okdas @bryanchriswhite @Olshansk