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

vocone: refactor Vocone struct as an extension of service.VocdoniService #1129

Merged
merged 4 commits into from
Oct 9, 2023

Conversation

altergui
Copy link
Contributor

@altergui altergui commented Oct 2, 2023

this way, the whole init phase is much similar to what node does, avoiding bugs
and making it easier to maintain in the future when features are added

this refactor was originated by a bug introduced when OffchainDataDownloader feature
was merged on node but not fully properly in voconed,
so voconed was not doing vs.DataDownloader.Start() that node does during init

the best solution was to make voconed call the same init function that node calls
srv.OffChainDataHandler()

@altergui altergui marked this pull request as draft October 2, 2023 15:07
@altergui altergui changed the title f/offchaindh fixes vocone: fix OffchainDataDownloader Oct 2, 2023
@altergui altergui self-assigned this Oct 2, 2023
@altergui
Copy link
Contributor Author

altergui commented Oct 2, 2023

i'm pushing this trivial fix, so that it can be used (or tested) already.

but i'd not merge this PR as-is, i'll try first to make a nicer refactor of voconed init and push that instead

@coveralls
Copy link

coveralls commented Oct 2, 2023

Pull Request Test Coverage Report for Build 6403401623

  • 55 of 67 (82.09%) changed or added relevant lines in 3 files are covered.
  • 12 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.003%) to 57.759%

Changes Missing Coverage Covered Lines Changed/Added Lines %
vocone/vocone.go 52 64 81.25%
Files with Coverage Reduction New Missed Lines %
vocone/vocone.go 12 61.64%
Totals Coverage Status
Change from base Build 6379751065: -0.003%
Covered Lines: 12625
Relevant Lines: 21858

💛 - Coveralls

@altergui altergui changed the title vocone: fix OffchainDataDownloader vocone: refactor Vocone struct as an extension of service.VocdoniService Oct 2, 2023
@altergui
Copy link
Contributor Author

altergui commented Oct 2, 2023

there you go, nicer refactor of voconed, that as a side effect fixes #1126 and prevents future issues like this when features are implemented

@altergui altergui marked this pull request as ready for review October 2, 2023 16:09
@altergui altergui requested a review from p4u October 2, 2023 16:09
@altergui
Copy link
Contributor Author

altergui commented Oct 2, 2023

i need to fix TestVocone but it's probably something trivial,
the PR can still be reviewed as is

i also thought of replacing that TestVocone with an actual integration test, since voconed binary (and docker image) is totally untested (and this created problems in the past for vocdoni-sdk CI)

@lucasmenendez
Copy link
Contributor

there you go, nicer refactor of voconed, that as a side effect fixes #1126 and prevents future issues like this when features are implemented

I was testing with this branch with two different tokens:

  • 0xBFAbdE619ed5C4311811cF422562709710DB587d (mainnet) -> ipfs://bafybeiehw3bldmap6buiscwbsys3bdcahpqt2xzugieu6ai2yq4ohijghq
  • 0xF530280176385AF31177D78BbFD5eA3f6D07488A (goerli) -> ipfs://bafybeiezzvgxwv6nykjkbcmqfd55g7smpilvj7nedx2crynyhtp2u2tx5a

And for me, the problem continues 🙁. The file is in the downloader queue but the census is not created.

@altergui
Copy link
Contributor Author

altergui commented Oct 3, 2023

thanks for the report,
i'd say it's related to the error that also happens in TestVocone

      e"missing modules attached for enabling handler censuses"

i'm looking into it

this way, the whole init phase is much similar to what `node` does, avoiding bugs
and making it easier to maintain in the future when features are added

this refactor was originated by a bug introduced when OffchainDataDownloader feature
was merged on `node` but not fully properly in `voconed`,
so `voconed` was not doing vs.DataDownloader.Start() that `node` does during init

the best solution was to make voconed call the same init function that node calls
srv.OffChainDataHandler()
@altergui altergui force-pushed the f/offchaindh-fixes branch 2 times, most recently from e9c3cc9 to 9091fe3 Compare October 3, 2023 12:29
Copy link
Member

@p4u p4u left a comment

Choose a reason for hiding this comment

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

Makes sense, thx

@altergui altergui force-pushed the f/offchaindh-fixes branch 3 times, most recently from 7c320fd to 9523a07 Compare October 3, 2023 13:14
Copy link
Contributor

@lucasmenendez lucasmenendez left a comment

Choose a reason for hiding this comment

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

The changes are OK for me, but testing the latest version it still fails to download censuses from IPFS, even with the same IPFS connect key in both services (census3 and vocone).

@altergui altergui merged commit 0a35fad into main Oct 9, 2023
10 checks passed
@altergui altergui deleted the f/offchaindh-fixes branch October 9, 2023 09:28
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.

4 participants