Skip to content

feat: peer id smart contract lookup #2

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

Merged
merged 34 commits into from
Mar 25, 2025

Conversation

NikolasHaimerl
Copy link
Contributor

@NikolasHaimerl NikolasHaimerl commented Mar 17, 2025

Smart Contract Client for IPNI Peer ID Resolution

This PR adds a new module that allows retrieving Filecoin index provider peer IDs from the on-chain smart contract. Key additions:

  • Implements getIndexProviderPeerIdFromSmartContract function to retrieve peer IDs for miner IDs
  • Adds comprehensive tests with mocked contract responses
  • Includes error handling for invalid miner IDs and network failures
  • Uses ethers.js for blockchain interaction for requests requests
  • Uses human readable ABIs

This shared module will be used by other Spark components to resolve miner IDs to their corresponding IPNI peer IDs.
I tried using a regular rpc function as we have for spark-checker but I kept getting corrupt data for the Peer Id, so I defaulted back to using ethers and leave the change to rpc usage for a later point in time.
Related to
CheckerNetwork/roadmap#250
CheckerNetwork/spark-checker#118
CheckerNetwork/spark-checker#117

@NikolasHaimerl NikolasHaimerl changed the title Nhaimerl peer id smart contract lookup feat: peer id smart contract lookup Mar 17, 2025
@juliangruber
Copy link
Member

juliangruber commented Mar 18, 2025

I tried using a regular rpc function as we have for spark-checker but I kept getting corrupt data for the Peer Id, so I defaulted back to using ethers and leave the change to rpc usage for a later point in time.

I assume the returned peer id is encoded and needs to be unpacked, rather than being corrupted. If JSON-RPC providers responded with corrupted data, we'd have a bigger issue. In any case, good to use a library then

Copy link
Contributor

@pyropy pyropy left a comment

Choose a reason for hiding this comment

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

It's looking great so far 👏🏻

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

@NikolasHaimerl How is the code in this pull request different from the implementation we have already shipped in spark-checker (via CheckerNetwork/spark-checker#117)?

I'd like to avoid second review of the code I have already approved.

@NikolasHaimerl
Copy link
Contributor Author

@NikolasHaimerl How is the code in this pull request different from the implementation we have already shipped in spark-checker (via CheckerNetwork/spark-checker#117)?

I'd like to avoid second review of the code I have already approved.

The code is very similar to that from CheckerNetwork/spark-checker#117, the only difference is the repo and the fact that we do not use zinnia for tests.

@NikolasHaimerl
Copy link
Contributor Author

NikolasHaimerl commented Mar 18, 2025

I tried using a regular rpc function as we have for spark-checker but I kept getting corrupt data for the Peer Id, so I defaulted back to using ethers and leave the change to rpc usage for a later point in time.

I assume the returned peer id is encoded and needs to be unpacked, rather than being corrupted. If JSON-RPC providers responded with corrupted data, we'd have a bigger issue. In any case, good to use a library then

I would assume so, yes. I tried a bunch of different decoding methods including decodeFunctionResult from ethers, decodeAbiParameters from viem, TextDecoder, hexToString from viem and manually eliminating empty bytes and converting the bytes to string through but I could not get it to work. The signature was easily extractable. I moved on because I did not want to postpone the release of this library due to that challenge.

Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

Code looks good to me, but RPC_AUTH needs to be passed by the library consumer, and RPC_URL should be changeable by the consumer.

@juliangruber
Copy link
Member

I personally prefer to document things as they are available, so for now I would document the constants ABI and the contract address. That is the extend of the public API after merging this PR. The reasoning is that if we say we'll document last, we're likely to miss things, we're less likely to want to spend time on documenting well, and more often than not, we'll end up not documenting it at all. It's similar to saying you'll add a test suite when the project is finished.

@NikolasHaimerl
Copy link
Contributor Author

I personally prefer to document things as they are available, so for now I would document the constants ABI and the contract address. That is the extend of the public API after merging this PR. The reasoning is that if we say we'll document last, we're likely to miss things, we're less likely to want to spend time on documenting well, and more often than not, we'll end up not documenting it at all. It's similar to saying you'll add a test suite when the project is finished.

I documented the two here: deaacf6

@juliangruber
Copy link
Member

I believe you committed this to the wrong branch?

@@ -13,7 +13,11 @@ jobs:
with:
node-version: 22
- run: npm ci
env:
RPC_AUTH: ${{secrets.GLIF_API_TOKEN}}
Copy link
Member

Choose a reason for hiding this comment

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

npm ci is installing dependencies; why does it need an RPC auth token? Can we remove this env: block?

Copy link
Member

Choose a reason for hiding this comment

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

npm ci is installing dependencies; why does it need an RPC auth token? Can we remove this env: block?

@NikolasHaimerl PTAL ☝🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed in 7263220

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Almost there!

I see two things remaining to address:

  • ci.yml comment below
  • missing documentation in README (did you perhaps accidentally push the commit to a different branch)?

@@ -13,7 +13,11 @@ jobs:
with:
node-version: 22
- run: npm ci
env:
RPC_AUTH: ${{secrets.GLIF_API_TOKEN}}
Copy link
Member

Choose a reason for hiding this comment

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

npm ci is installing dependencies; why does it need an RPC auth token? Can we remove this env: block?

@NikolasHaimerl PTAL ☝🏻

Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

One comment + missing docs (wrong branch)

@NikolasHaimerl
Copy link
Contributor Author

I believe you committed this to the wrong branch?

Yes, too many working branches on this library I feel. The change is in 66c0c16

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM after you address Julian's two comments above 👏🏻

NikolasHaimerl and others added 3 commits March 24, 2025 17:41
Co-authored-by: Julian Gruber <[email protected]>
Co-authored-by: Julian Gruber <[email protected]>
Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

it's shipping time

@juliangruber
Copy link
Member

(as soon as CI passes)

@NikolasHaimerl NikolasHaimerl merged commit 2f57507 into main Mar 25, 2025
4 checks passed
@NikolasHaimerl NikolasHaimerl deleted the nhaimerl-peer-id-smart-contract-lookup branch March 25, 2025 07:45
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