Skip to content

update wait time#278

Merged
tzdybal merged 7 commits intoevstack:mainfrom
toanngosy:aggregation-loop
Jan 23, 2023
Merged

update wait time#278
tzdybal merged 7 commits intoevstack:mainfrom
toanngosy:aggregation-loop

Conversation

@toanngosy
Copy link
Copy Markdown
Contributor

@toanngosy toanngosy commented Feb 5, 2022 •
edited
Loading

Implements the wait time as required in issue #195 and issue #196

@toanngosy toanngosy mentioned this pull request Feb 5, 2022
Closed
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Feb 5, 2022 •
edited
Loading

Codecov Report

Merging #278 (e61a464) into main (1a42c63) will decrease coverage by 0.04%.
The diff coverage is 35.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
- Coverage   58.33%   58.28%   -0.05%     
==========================================
  Files          44       44              
  Lines        6780     6794      +14     
==========================================
+ Hits         3955     3960       +5     
- Misses       2283     2289       +6     
- Partials      542      545       +3
Impacted Files Coverage Δ
block/manager.go 60.16% <35.71%> (-1.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a42c63...e61a464. Read the comment docs.

This was linked to issues Feb 6, 2022
Maintain correct block time after restarting #195
Closed
Genesis time is not used #196
Closed
tzdybal
tzdybal reviewed Feb 6, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

Thanks for your submission!

I left a some comments on refactoring to simplify the code.

Comment thread block/manager.go Outdated
Comment thread block/manager.go Outdated
Comment thread block/manager.go Outdated
@toanngosy
Copy link
Copy Markdown
Contributor Author

toanngosy commented Feb 7, 2022

Thanks for your submission!

I left a some comments on refactoring to simplify the code.

Thanks @tzdybal , let me know if the new code looks good to you.

@toanngosy toanngosy requested a review from tzdybal February 7, 2022 22:49
tzdybal
tzdybal reviewed Feb 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

Left a suggestion to simplify the code.

Comment thread block/manager.go Outdated
@toanngosy toanngosy requested a review from tzdybal February 8, 2022 01:28
@adlerjohn
Copy link
Copy Markdown
Contributor

adlerjohn commented Feb 23, 2022

Is there anything holding up this PR?

@tzdybal
Copy link
Copy Markdown
Contributor

tzdybal commented Feb 23, 2022

Is there anything holding up this PR?

Lack of tests.

@adlerjohn adlerjohn marked this pull request as draft February 24, 2022 00:16
@toanngosy
Copy link
Copy Markdown
Contributor Author

toanngosy commented Feb 24, 2022

I'll push a test this week

toanngosy
toanngosy commented Mar 5, 2022
View reviewed changes
Comment thread block/manager.go
toanngosy
toanngosy commented Mar 5, 2022
View reviewed changes
Comment thread rpc/client/client_test.go Outdated
tzdybal
tzdybal reviewed Mar 6, 2022
View reviewed changes
Comment thread rpc/client/client_test.go Outdated
Comment thread rpc/client/client_test.go Outdated
Comment thread rpc/client/client_test.go Outdated
Comment thread rpc/client/client_test.go Outdated
@tzdybal tzdybal self-assigned this May 6, 2022
@tzdybal
Copy link
Copy Markdown
Contributor

tzdybal commented Jul 30, 2022

Hey @toanngosy! any updates on this? it's super close to be finished, I would love to merge it.

@tzdybal tzdybal force-pushed the aggregation-loop branch from c6485b2 to b5d13d7 Compare July 30, 2022 10:03
@tzdybal
Copy link
Copy Markdown
Contributor

tzdybal commented Jul 30, 2022

I rebased the changes, everything compiles. Failing test should be changed anyways.

@tzdybal tzdybal marked this pull request as ready for review January 19, 2023 08:35
@tzdybal tzdybal requested review from a team and S1nus and removed request for a team, adlerjohn and liamsi January 19, 2023 08:36
@tzdybal
Copy link
Copy Markdown
Contributor

tzdybal commented Jan 19, 2023

I rebased this PR + fixed the test so it tests only what is needed.

Additionally found a bug; created #699.

nashqueue
nashqueue suggested changes Jan 19, 2023
View reviewed changes
Comment thread block/manager.go
@tzdybal tzdybal requested a review from nashqueue January 19, 2023 12:05
nashqueue
nashqueue previously approved these changes Jan 19, 2023
View reviewed changes
@nashqueue
Copy link
Copy Markdown
Contributor

nashqueue commented Jan 19, 2023

Why is build-mockserv docker build skipped ?

@tzdybal tzdybal enabled auto-merge (squash) January 19, 2023 13:48
@tzdybal
Copy link
Copy Markdown
Contributor

tzdybal commented Jan 19, 2023

Why is build-mockserv docker build skipped ?

PR from fork; it has no access rights to publish docker images, so it's skipped.

gupadhyaya
gupadhyaya reviewed Jan 20, 2023
View reviewed changes
Comment thread block/manager.go Outdated
@adlerjohn adlerjohn requested review from tzdybal and removed request for S1nus January 23, 2023 02:26
@tzdybal tzdybal merged commit a972fac into evstack:main Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Manav-Aggarwal Manav-Aggarwal Manav-Aggarwal approved these changes

+3 more reviewers

@tzdybal tzdybal tzdybal approved these changes

@gupadhyaya gupadhyaya gupadhyaya approved these changes

@nashqueue nashqueue nashqueue approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tzdybal tzdybal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Genesis time is not used Maintain correct block time after restarting

7 participants

@toanngosy @codecov-commenter @adlerjohn @tzdybal @nashqueue @gupadhyaya @Manav-Aggarwal