Implement missing gRPC filters #4462

aoudiamoncef · 2023-10-09T16:03:21Z

document all added functions
try in sandbox /simulation/labnet
- if part of node-launch, checked using the resync_check flag
unit tests on the added/changed features
- make tests compile
- make tests pass
add logs allowing easy debugging in case the changes caused problems
if the API has changed, update the API specification

We have some TODOs in massa-proto and make it more defensive to avoid flooding the clients.

// NewSlotExecutionOutputs Filter
message NewSlotExecutionOutputsFilter {
  // Filter
  oneof filter {
    //TODO add ALL to receive all the slot execution outputs. Same for all
    // Execution output status
    massa.model.v1.ExecutionOutputStatus status = 1;
    // Slot range
    massa.model.v1.SlotRange slot_range = 2;
    //TODO by default nothing is returned, we should add a filter to receive all the slot execution outputs ALL
    // AsyncPoolChangesFilter
    AsyncPoolChangesFilter async_pool_changes_filter = 3;
	...
  }
}

// AsyncPoolChangesFilter
message AsyncPoolChangesFilter {
  // Filter
  oneof filter {
    //TODO to be replaced by ALL
    // Do not return any message
    massa.model.v1.Empty none = 1;
    ```
    
The changes above are breaking the gRPC API and should be continued when builders requires those features and are ready to upgrade.

massa-grpc/src/stream/new_slot_execution_outputs.rs

aoudiamoncef · 2023-10-11T13:07:33Z

massa-grpc/src/stream/new_slot_execution_outputs.rs

+                        return false;
+                    };
+                }
+                //TODO to be confirmed


@AurelienFT could you confirm this information about emitter_address ?

No the emitter address should be the back of the callstack. But currently in the code it's inverted see : #4409 but we are not correcting it now because it could break too much projects : #4422 (comment) (I'm not fully agree with this but not the subject) So I suggest that you match the wrong behaviour and use .front() like here

aoudiamoncef · 2023-10-11T13:09:16Z

massa-grpc/src/stream/new_slot_execution_outputs.rs

+                                }
+                            }
+                            //TODO to be confirmed
+                            if let Some(handlers) = &async_pool_changes_filter.handlers {


@AurelienFT same here about handler

I have renamed handler to function because it was his real usage. So yes handler before == function now.

aoudiamoncef · 2023-10-11T13:11:06Z

massa-grpc/src/stream/new_slot_execution_outputs.rs

+                                        }
+                                    }
+                                    //TODO to be confirmed
+                                    massa_ledger_exports::SetOrKeep::Keep => {


@AurelienFT if It's a Keep, we can't compare to anything, should we do something or act like there is nothing to compare ?

If a change is here with a function set to Keep then there is at least another change that is here with a Set and so the choice of returning it or not should depends on the choice on the field that have the Set

We'll take a decision when this PR will be prioritised.

modship

can u make good unit test for new_slot_execution_output

aoudiamoncef · 2023-10-12T21:19:04Z

can u make good unit test for new_slot_execution_output

This won't fit in one big test, we tons of case to cover.... But i'm trying to reuse existing test tools functions to generate test data to have more accurate tests. Stay tuned

aoudiamoncef · 2023-10-16T16:05:28Z

I made a small refactoring in bootstrap tests to be able to extract the tools module and make it public.

aoudiamoncef · 2023-10-17T15:52:35Z

@modship I've added some basic tests which covers filters usage. But there if a lot of combinations to test

massa-bootstrap/src/tests/scenarios.rs

modship · 2023-10-18T15:59:14Z

massa-grpc/src/tests/stream.rs

+    filters.push(filter);
+
+    tx_request
+        .send(NewSlotExecutionOutputsRequest { filters })


to improve readability please remove the vector filters and create the vec here

modship · 2023-10-18T15:59:44Z

massa-grpc/src/tests/stream.rs

+        .unwrap()
+        .state_changes
+        .unwrap();
+    assert!(state_changes.async_pool_changes.len() == 2);


use assert_eq! when you check specific value

modship · 2023-10-18T15:59:51Z

massa-grpc/src/tests/stream.rs

+        .state_changes
+        .unwrap();
+    assert!(state_changes.async_pool_changes.len() == 2);
+    assert!(state_changes.executed_ops_changes.len() == 1);


use assert_eq! when you check specific value

aoudiamoncef changed the title ~~Implement missing filter parsing~~ Implement missing gRPC filters Oct 9, 2023

modship reviewed Oct 9, 2023

View reviewed changes

massa-grpc/src/stream/new_slot_execution_outputs.rs Outdated Show resolved Hide resolved

modship reviewed Oct 9, 2023

View reviewed changes

massa-grpc/src/stream/new_slot_execution_outputs.rs Outdated Show resolved Hide resolved

aoudiamoncef force-pushed the feature/add-missing-filters branch from 1ed67b6 to 1e86981 Compare October 10, 2023 13:46

aoudiamoncef marked this pull request as ready for review October 10, 2023 17:03

aoudiamoncef requested a review from modship October 11, 2023 13:06

aoudiamoncef commented Oct 11, 2023

View reviewed changes

aoudiamoncef force-pushed the feature/add-missing-filters branch from 6d519da to 85a8a1a Compare October 11, 2023 13:11

aoudiamoncef requested a review from AurelienFT October 11, 2023 13:12

modship requested changes Oct 11, 2023

View reviewed changes

aoudiamoncef force-pushed the feature/add-missing-filters branch from 85a8a1a to 736e5f5 Compare October 12, 2023 22:12

aoudiamoncef requested a review from modship October 17, 2023 15:53

aoudiamoncef added 11 commits October 17, 2023 21:26

feat: implement missing filter parsing

3db8c6f

feat: implement some filters logic

4115b5d

fix: nested filters definitions

58339c4

feat: add execution_event_filter

dae5f9a

feat: add AsyncPoolChanges filters

0ee1d8f

refactor: start generating test data

6619600

refactor: bootstrap test tools

75583fe

feat: add event store rand

9044d94

refactor: add some basic tests

338c753

refactor: enhanced async pool test

52082bd

fix: rebasing issues

00c66b8

aoudiamoncef force-pushed the feature/add-missing-filters branch from cd83ef2 to 00c66b8 Compare October 17, 2023 19:43

fix: mock import issue

22b1b40

damip reviewed Oct 18, 2023

View reviewed changes

massa-bootstrap/src/tests/scenarios.rs Outdated Show resolved Hide resolved

aoudiamoncef added blocked Issues that can't be done for now. enhancement New feature or request labels Oct 18, 2023

aoudiamoncef marked this pull request as draft October 18, 2023 10:22

refactor: add more tests

e6b8c90

modship reviewed Oct 18, 2023

View reviewed changes

modship linked an issue Oct 19, 2023 that may be closed by this pull request

API unit tests #4426

Closed

70 tasks

Leo-Besancon changed the base branch from main to securnet_wip November 6, 2023 15:01

Leo-Besancon changed the base branch from securnet_wip to buildnet_wip November 8, 2023 15:07

Leo-Besancon changed the base branch from buildnet_wip to pre_mainnet_wip November 28, 2023 08:41

Base automatically changed from pre_mainnet_wip to main May 28, 2024 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement missing gRPC filters #4462

Implement missing gRPC filters #4462

aoudiamoncef commented Oct 9, 2023 •

edited

Loading

aoudiamoncef Oct 11, 2023 •

edited

Loading

AurelienFT Oct 18, 2023

aoudiamoncef Oct 11, 2023 •

edited

Loading

AurelienFT Oct 18, 2023

aoudiamoncef Oct 11, 2023 •

edited

Loading

AurelienFT Oct 18, 2023

aoudiamoncef Oct 18, 2023

modship left a comment

aoudiamoncef commented Oct 12, 2023 •

edited

Loading

aoudiamoncef commented Oct 16, 2023 •

edited

Loading

aoudiamoncef commented Oct 17, 2023

modship Oct 18, 2023

modship Oct 18, 2023

modship Oct 18, 2023

Implement missing gRPC filters #4462

Are you sure you want to change the base?

Implement missing gRPC filters #4462

Conversation

aoudiamoncef commented Oct 9, 2023 • edited Loading

aoudiamoncef Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

AurelienFT Oct 18, 2023

Choose a reason for hiding this comment

aoudiamoncef Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

AurelienFT Oct 18, 2023

Choose a reason for hiding this comment

aoudiamoncef Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

AurelienFT Oct 18, 2023

Choose a reason for hiding this comment

aoudiamoncef Oct 18, 2023

Choose a reason for hiding this comment

modship left a comment

Choose a reason for hiding this comment

aoudiamoncef commented Oct 12, 2023 • edited Loading

aoudiamoncef commented Oct 16, 2023 • edited Loading

aoudiamoncef commented Oct 17, 2023

modship Oct 18, 2023

Choose a reason for hiding this comment

modship Oct 18, 2023

Choose a reason for hiding this comment

modship Oct 18, 2023

Choose a reason for hiding this comment

aoudiamoncef commented Oct 9, 2023 •

edited

Loading

aoudiamoncef Oct 11, 2023 •

edited

Loading

aoudiamoncef Oct 11, 2023 •

edited

Loading

aoudiamoncef Oct 11, 2023 •

edited

Loading

aoudiamoncef commented Oct 12, 2023 •

edited

Loading

aoudiamoncef commented Oct 16, 2023 •

edited

Loading