Skip to content

Conversation

@taslimmuhammed
Copy link
Contributor

Fixes #361
tried fixing only disputes[caused a chain reaction], had to fix all related too,

  • didn't rename the pipe to handle[Although they are now handles, serves purpose of a pipe]
  • added wait for many tests in "common" dir.
  • removed error check .map_err(|e| CheckError::Retryable(e.into()))? in tap-agent signature

if any of the above violates current code/is wrong do tell, I'll fix, also there are so many unnecessary comments, I'll remove after review

@gusinacio
Copy link
Contributor

Wow, I didn't know it would cause this chain reaction. It turned out to be bigger than I thought, sorry for that.

@taslimmuhammed
Copy link
Contributor Author

No, I'm sorry I messed up, you only asked for changing the dispute variable right?, misunderstood, changed all the eventual variables in the errored files, if for now, you want change only on disputes do tell, I'll push it seperately.

@gusinacio
Copy link
Contributor

No, I'm sorry I messed up, you only asked for changing the dispute variable right?, misunderstood, changed all the eventual variables in the errored files, if for now, you want change only on disputes do tell, I'll push it seperately.

Do you know if it's possible to split into 4 different PRs (assuming all of the PRs are gonna build/test correctly)?

It's currently too big and I'm scared of miss anything. In case this is not possible just let me know and I'll try to review carefully this big PR.

@gusinacio gusinacio self-requested a review October 14, 2024 16:11
@taslimmuhammed
Copy link
Contributor Author

Sure, I'll do that, but I can only do that tomorrow, came out for a small interview .

Copy link
Contributor

@gusinacio gusinacio left a comment

Choose a reason for hiding this comment

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

Hey, this is just a mini-review about the current work. It's in the right direction!

@coveralls
Copy link

coveralls commented Oct 16, 2024

Pull Request Test Coverage Report for Build 11407487878

Details

  • 62 of 69 (89.86%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.07%) to 71.035%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/src/attestations/signers.rs 42 45 93.33%
common/src/attestations/dispute_manager.rs 20 24 83.33%
Files with Coverage Reduction New Missed Lines %
common/src/attestations/signers.rs 1 95.29%
Totals Coverage Status
Change from base Build 11393425137: 0.07%
Covered Lines: 4640
Relevant Lines: 6532

💛 - Coveralls

@taslimmuhammed
Copy link
Contributor Author

taslimmuhammed commented Oct 16, 2024

Pull Request Test Coverage Report for Build 11364651032

Details

* **248** of **302**   **(82.12%)**  changed or added relevant lines in **17** files are covered.

* **4** unchanged lines in **2** files lost coverage.

* Overall coverage increased (+**0.08%**) to **70.629%**

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/src/tap.rs 0 2 0.0%
common/src/tap/checks/sender_balance_check.rs 0 2 0.0%
tap-agent/src/agent/sender_account.rs 100 102 98.04%
common/src/tap/checks/allocation_eligible.rs 0 3 0.0%
common/src/attestations/dispute_manager.rs 23 27 85.19%
common/src/escrow_accounts.rs 15 20 75.0%
common/src/indexer_service/http/request_handler.rs 0 10 0.0%
common/src/allocations/monitor.rs 0 26 0.0%
Files with Coverage Reduction New Missed Lines %
common/src/allocations/monitor.rs 1 0.0%
tap-agent/src/agent/sender_account.rs 3 92.7%
Totals Coverage Status
Change from base Build 11351305070: 0.08%
Covered Lines: 4516
Relevant Lines: 6394

💛 - Coveralls

hi, I'm little new to this, does this mean you have reviewed the most of code?, cause I was going to just limit the PR to disputes_manger as of now

@gusinacio
Copy link
Contributor

hi, I'm little new to this, does this mean you have reviewed the most of code?, cause I was going to just limit the PR to disputes_manger as of now

Nope, this is just a verification check to understand test coverage of your PR. This means what's the percentage of your code is being tested by tests.

You don't need to worry about it.

@taslimmuhammed
Copy link
Contributor Author

Hi, please have a look, it was responding to both variable changes when I tested in a local program.

Copy link
Contributor

@gusinacio gusinacio left a comment

Choose a reason for hiding this comment

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

Hey, I loved your solution. Here are just some small changes and questions.

gusinacio
gusinacio previously approved these changes Oct 18, 2024
Copy link
Contributor

@gusinacio gusinacio left a comment

Choose a reason for hiding this comment

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

LGTM. Could you fix the issues with CI? We had a mini refactor updating the imports for Address to use alloy::primitives, I guess your merge didn't handle it.

suchapalaver
suchapalaver previously approved these changes Oct 18, 2024
@taslimmuhammed
Copy link
Contributor Author

LGTM. Could you fix the issues with CI? We had a mini refactor updating the imports for Address to use alloy::primitives, I guess your merge didn't handle it.

sure, I had to resolve last conflict on github so couldn't run commands, BTW:-

  • cargo clippy --all-targets --all-features # for clippy
  • cargo fmt # for fmt
  • cargo sqlx prepare --workspace -- --all-targets # for SQL offline mode
    even after running all of this some PR checks usually fails, am I doing it wrong, or missing any commands?

@taslimmuhammed
Copy link
Contributor Author

Thank you for taking the time to review and approve my PR. I really appreciate your feedback and support in getting this merged!

@gusinacio
Copy link
Contributor

even after running all of this some PR checks usually fails, am I doing it wrong, or missing any commands?

We have two tests that usually fail:

  • Lint PR: we use conventional commits, if your commit messages are not like that, it'll fail. Usually, I squash and set the correct message for the PR. If you start using them, they'll start to pass.
  • Build and upload Docker image: it's a problem on our end, we shouldn't try to push docker images in case it's an external contributor.

Other than those two checks, all of the others should pass correctly.

@taslimmuhammed taslimmuhammed dismissed stale reviews from suchapalaver and gusinacio via 2353f73 October 18, 2024 16:17
Comment on lines 7 to 10
use thegraph_core::Address;
use tokio::sync::watch::{self, Receiver};
use tokio::time::{self, sleep};
use alloy::primitives::Address;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use use alloy::primitives::Address; instead of use thegraph_core::Address;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh , sorry

Copy link
Contributor

@gusinacio gusinacio left a comment

Choose a reason for hiding this comment

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

LGTM, thank you so much for the clean and beautiful code.

@gusinacio gusinacio changed the title Drop eventuals in favor of Tokio watch + timers for disputes refactor: replace eventuals with Tokio watch for disputes Oct 18, 2024
@gusinacio gusinacio merged commit 817703e into graphprotocol:main Oct 18, 2024
7 of 10 checks passed
@taslimmuhammed
Copy link
Contributor Author

Really humbled by your kind words, BTW could you assign attestation_signers too to me

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.

Drop eventuals in favor of Tokio watch + timers for disputes

4 participants