Skip to content

Conversation

@YashBit
Copy link
Contributor

@YashBit YashBit commented Oct 15, 2024

Solve for issue: #115

@YashBit
Copy link
Contributor Author

YashBit commented Oct 15, 2024

@gusinacio

Would appreciate your review.

Updated where I found variables using the hardcoded values of eip712_domain!

Would appreciate some patience if my corrections are not fully there, I am still new to Rust. Hence, picking a good-first-issue.

@gusinacio
Copy link
Contributor

Hello @YashBit, thank you for the contribution. It looks good to me, could you check our CI jobs, there are a few failing.

@gusinacio gusinacio self-requested a review October 15, 2024 13:26
@coveralls
Copy link

coveralls commented Oct 15, 2024

Pull Request Test Coverage Report for Build 11347367932

Details

  • 5 of 9 (55.56%) changed or added relevant lines in 3 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 70.508%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/src/indexer_service/http/indexer_service.rs 0 4 0.0%
Files with Coverage Reduction New Missed Lines %
common/src/tap/checks/deny_list_check.rs 1 98.21%
Totals Coverage Status
Change from base Build 11262741398: -0.03%
Covered Lines: 4485
Relevant Lines: 6361

💛 - Coveralls

@gusinacio
Copy link
Contributor

Hey @YashBit, It looks like Clippy is still failing. Try running it with cargo clippy --all-targets --all-features.

@YashBit
Copy link
Contributor Author

YashBit commented Oct 15, 2024

@gusinacio Done. If PR works, please do assign me the feature issue that I have requested.

@gusinacio
Copy link
Contributor

Hey, @YashBit, it's still failing

@gusinacio
Copy link
Contributor

could you rebase and fix the conflict? I cannot run the CI without it.

@YashBit
Copy link
Contributor Author

YashBit commented Oct 16, 2024

@gusinacio There seems to be some problem with the CI checks.

Please could you suggest the best course of action?

I apologize, but I have run all the commands from my end, and resolved all errors.

@gusinacio
Copy link
Contributor

So, I recommend you performing:

git remote add graph [email protected]:graphprotocol/indexer-rs.git
git pull graph main --rebase

Try to solve conflicts that may emerge using git tool.

After that, you probably need to run:

cargo clippy --all-targets --all-features

And fix them one by one until it's not showing any errors. Let me know how that goes.

@gusinacio
Copy link
Contributor

Also, our CI uses Rust 1.80.

Maybe you need to set your rust version to it:

rustup install 1.80
rustup override set 1.80
cargo version

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.

Wow, that was a long journey hahaha. But we finally got to a point where it's working.

Thank you for your contribution!

@gusinacio gusinacio changed the title refactor: tap_core::tap_eip712_domain refactor: use tap_core::tap_eip712_domain Oct 22, 2024
@gusinacio gusinacio merged commit 782dd67 into graphprotocol:main Oct 22, 2024
7 of 10 checks passed
@gusinacio gusinacio linked an issue Oct 22, 2024 that may be closed by this pull request
@YashBit
Copy link
Contributor Author

YashBit commented Oct 22, 2024

@gusinacio Sorry about that. It was all in the rustc version.

Thanks for your help. Will it be possible for us to hop on a short call online? I have talked to some people at The Graph. Would love to draft a plan of contributions.

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.

Use hardcoded Tap Domain Separator from tap-core

3 participants