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

refactor: use tokio::watch for escrow_accounts #413

Merged
merged 12 commits into from
Nov 4, 2024

Conversation

taslimmuhammed
Copy link
Contributor

Fixes #365

@taslimmuhammed
Copy link
Contributor Author

terribly sorry for the delay, I first tried the fix using Receiver<Option>, although it was not required, was reluctant to skip some error checks, please have a look when you're free also please tell me if any changes are required.

@coveralls
Copy link

coveralls commented Oct 26, 2024

Pull Request Test Coverage Report for Build 11641140606

Details

  • 94 of 112 (83.93%) changed or added relevant lines in 15 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.1%) to 74.61%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/src/tap.rs 0 1 0.0%
common/src/indexer_service/http/request_handler.rs 0 2 0.0%
common/src/tap/checks/sender_balance_check.rs 0 2 0.0%
common/src/indexer_service/http/indexer_service.rs 0 3 0.0%
tap-agent/src/agent.rs 0 3 0.0%
tap-agent/src/agent/sender_accounts_manager.rs 21 28 75.0%
Files with Coverage Reduction New Missed Lines %
common/src/escrow_accounts.rs 3 88.79%
Totals Coverage Status
Change from base Build 11637439605: -0.1%
Covered Lines: 5360
Relevant Lines: 7184

💛 - Coveralls

common/src/escrow_accounts.rs Outdated Show resolved Hide resolved
@taslimmuhammed
Copy link
Contributor Author

hi, updated with initial accounts, please have a look when you're free, tell me if any more changes are required

Copy link
Member

@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 all really nice. Just two missed comments.

Now that you removed all eventuals, could you also remove it from the dependencies? Let me know how it goes, I suppose it's not used anywhere anymore.

common/src/indexer_service/http/indexer_service.rs Outdated Show resolved Hide resolved
common/src/indexer_service/http/indexer_service.rs Outdated Show resolved Hide resolved
@taslimmuhammed
Copy link
Contributor Author

Thanks for the review, Eventual is still used in couple of places for other variables, I'll change it and push asap.

@taslimmuhammed
Copy link
Contributor Author

BTW, I have send a connection request on linkedin please accept😁

@taslimmuhammed
Copy link
Contributor Author

Hi, the last commit is not the fix. I had to push due run on diff system, I'll let you know when it's ready

@taslimmuhammed
Copy link
Contributor Author

Hi sir, it's ready, please review when you're free, also tell me if any changes are required.

Copy link
Member

@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'm sorry that you had to upgrade two more eventuals here, I thought that those 4 were the only ones.

As you may suspect, I don't like eventuals that contains None or default values. IMO this is the biggest flaw with eventuals.

common/src/subgraph_client/client.rs Outdated Show resolved Hide resolved
common/src/subgraph_client/monitor.rs Outdated Show resolved Hide resolved
common/src/subgraph_client/monitor.rs Outdated Show resolved Hide resolved
tap-agent/src/tap/context/checks/allocation_id.rs Outdated Show resolved Hide resolved
@taslimmuhammed
Copy link
Contributor Author

Thanks for the review I'll send the update asap.

@gusinacio
Copy link
Member

Hey, quick thing: I added a module called watcher with some helper functions. Feel free to use them.

@taslimmuhammed
Copy link
Contributor Author

sure, I'll check it out

@taslimmuhammed
Copy link
Contributor Author

Hi, checked out new module, and it's lit[🔥], highly needed, repetitive components was getting little weird, I think it's better to change that in a new PR, cause this PR itself is little lengthier besides we should update all channels.

@taslimmuhammed
Copy link
Contributor Author

Hi, update the code, please have a look when you're free, it have gotten little longer, but I hope the size doesn't matter cause most of it is already reviewed, important change is that, dummy_url cannot be used in the test anymore cause we don't have empty values and request cant go wrong while creation. Also tell me if any changes are required.

@gusinacio
Copy link
Member

Hey @taslimmuhammed, actually, it got way harder to review. Sorry, it was a bad decision to remove the all eventual. Can we split this PR in:

  • escrow-accounts
  • allocation-id
  • subgraph-monitor

Also, if I'm missing any eventual, do it in a separate PR.

We were almost ready with this but the newer changes were too much, I'm sorry again.

@gusinacio
Copy link
Member

gusinacio commented Oct 30, 2024

Also, add me in Discord: gustavoinacio

@taslimmuhammed
Copy link
Contributor Author

It's ok, I'll send the commit asap, BTW it's better to convert all elements to use watcher in a diff PR right?

@taslimmuhammed
Copy link
Contributor Author

Hi, it's ready, have a look when you're free, also tell me if any more changes are needed

@taslimmuhammed
Copy link
Contributor Author

taslimmuhammed commented Oct 31, 2024

Hey @taslimmuhammed, actually, it got way harder to review. Sorry, it was a bad decision to remove the all eventual. Can we split this PR in:

* escrow-accounts

* allocation-id

* subgraph-monitor

Also, if I'm missing any eventual, do it in a separate PR.

We were almost ready with this but the newer changes were too much, I'm sorry again.

me too never thought changing 2 small eventuals, would become this large😂, it truly caused a chain reaction, they are utilized in almost everywhere.

@taslimmuhammed
Copy link
Contributor Author

Hey this is all really nice. Just two missed comments.

Now that you removed all eventuals, could you also remove it from the dependencies? Let me know how it goes, I suppose it's not used anywhere anymore.

Hi, please note this is the same code reviewed above, I just moved backwards. Also regarding other updates, if it's ok, no need of new issue, I'll send new PR for each eventuals and watcher one by one, have everything ready.

Copy link
Member

@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.

Thank you, way easier to review. There's only one function that we can use it and all the rest LGTM.

common/src/escrow_accounts.rs Outdated Show resolved Hide resolved
Comment on lines 144 to 146
let escrow_accounts = state.escrow_accounts.clone();
let sender = escrow_accounts
.borrow()
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need to clone here.

tap-agent/src/agent/sender_account.rs Show resolved Hide resolved
@taslimmuhammed
Copy link
Contributor Author

thanks, i'll send the fixes asap

@taslimmuhammed
Copy link
Contributor Author

Hi, it's ready.

Copy link
Member

@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.

thank you! This is the last review

common/src/escrow_accounts.rs Outdated Show resolved Hide resolved
@taslimmuhammed
Copy link
Contributor Author

Hi, updated[✌️]

Copy link
Member

@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

@gusinacio gusinacio changed the title Drop eventuals in favor of Tokio watch + timers for escrow_accounts refactor: use tokio::watch for escrow_accounts Nov 4, 2024
@gusinacio gusinacio merged commit 5fa85ac into graphprotocol:main Nov 4, 2024
9 of 10 checks passed
@taslimmuhammed
Copy link
Contributor Author

thanks alot sir[🫂]

@gusinacio
Copy link
Member

Thank you @taslimmuhammed!

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 escrow_accounts
3 participants