-
Notifications
You must be signed in to change notification settings - Fork 0
x402 Provider Support #98
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
Conversation
Checkpoint commit, some jank when calling out to the facilitator that I can't figure out easily
checkpoint commit. lfg
it works (on testnet at least)
Ok seems good, but need to test this with Mainnet Base / USDC (should be fine but there's some fiddly stuff with EIP-3009). Also need to ensure that a migration will be fine.
|
Probably good to support the additional output schema requested here: https://www.x402scan.com/resources/register I'll take care of that. @Gohlub it would be good if you added an |
provider/provider/src/lib.rs
Outdated
| info!("x402 endpoint called"); | ||
|
|
||
| // ===== CHECK FOR X-PAYMENT HEADER ===== | ||
| let x_payment_header = APP_HELPERS.with(|helpers| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should not use APP_HELPERS, iirc its used by the macro internally so we should have some code in process-lib that enables getting the header, same comment for wherever we use it to fetch the relevant context about the http request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorted
provider/provider/src/lib.rs
Outdated
| pub spent_tx_hashes: Vec<String>, | ||
| // TODO: Replace with persistent storage for production - tracks used payment nonces to prevent replay attacks | ||
| #[serde(skip)] | ||
| pub used_nonces: HashSet<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can put this in our key-value store process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoiding to modify the state also saves us from writing more state migration logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though since we need the new note, we might have to do it anyways lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of annoying that we have to write migration logic for every single note we try to add (or, presumably, remove). Is there some obvious way around this or do we just have to suck it up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's just about state persistence: it is plausible we can move provider metadata to not live in the state as it's all public info anyways, though imo changing the structure of notes on a protocol level should only happen on higher version bumps: because of the distributed nature of our deployments, for every such change we have to add extra logic that nudges, and correctly tracks what is missing in the client (provider in our case) to get on their node and sign extra transactions, I had to do such extra work for the is_live note from some time ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
furthermore, we have restrictions on what notes are allowed in the namespace so that's yet another dependency we need to keep a track off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: check if #[serde(skip)] means this won't break state and cause a panic.
Generally, we should test the upgrade path, @Gohlub only flagged this as a potential state troll but who knows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be clear it isn't super important to keep track of the nonces. EIP 3009 kind of does it for us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rid of it.
| }; | ||
|
|
||
| // Validate protocol version | ||
| if payment_payload.protocol_version != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless there's a specific reason we're doing the validation here, it makes more sense to do it sooner rather than later: if the client is using an outdated protocol version (not sure if payment would go through in the first place), we don't want them to get rejected after being told to send the payment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inspecting the code further, I see this comes from 'parse_x_payment_header', which is the second step so it actually does make sense to do this here, I am not sure exactly about how x402 is set up but I feel like validating the protocol version should be done sooner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a huge deal because of how EIP-3009 works — a modified malicious client could maybe steal user funds by counterfeiting everything after receiving the signature on false pretenses, but I'm not sure about that and if it is the case it's a problem across all x402 clients not just this implementation.
| payment_payload.scheme, payment_payload.network); | ||
| method | ||
| }, | ||
| None => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defensive validation (which is okay), but unclear why this would be None given the contents of build_payment_requirements as those fields are populated by constants "exact" and "X402_PAYMENT_NETWORK"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wontfix
| return Ok("".to_string()); | ||
| } | ||
|
|
||
| info!("Payment verified for payer: {}", verify_result.payer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info! will be propagated to hypergrid_logger, so we should send this log after replay protection check
also, we should update the hypergrid_logger process to include x402 payments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wontfix
|
haven't ran the code but it looks good (will require small modifications), will probably want to abstract the logic from the handler and put it in util.rs (just for better readability) |
I might just be spitballing but I need to check in with Nick since I get the feeling that this might not work for self-hosted indirect nodes. |
provider/provider/src/lib.rs
Outdated
| /// Build PaymentRequirements structure from provider and resource URL | ||
| fn build_payment_requirements(provider: &RegisteredProvider, resource_url: &str) -> PaymentRequirements { | ||
| // Convert USDC price to atomic units (6 decimals) | ||
| let max_amount_atomic = ((provider.price * 1_000_000.0) as u64).to_string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had flagged this previously but just got lazy about it, will fix.
just checked in with Nick: the note should be optional as it would otherwise doxx indirect nodes |
|
Do you mind sharing more info about sepolia/base discrepency RE EIP-3009? |
|
RE sepolia/base there's a few: EIP-3009 expects a network name, token addresses, and then crucially the onchain name of the token matters. On Base it is "USD Coin" and on Sepolia it is "USDC" (or maybe the other way around I can't remember) and you need to specify that. RE doxxing indirect nodes: does that mean that indirect nodes can never field inbound HTTP requests? I'm not understanding what the problem here is. If it's really just "doxxing" (in the sense that you know what URL someone hosts their node at) it's a nonissue since you don't have to use Hypergrid if you're that sensitive about opsec. If there's an actual risk that's different, but I can't imagine how that would be the case. |
it's doxxing in the sense of that providers will post their home IP address on-chain, and I think it might even require them to fiddle with their routers to expose ports to the internet (it's why the node boot-sequence defaults to indirect and warns you do leave it like that unless you know what you're doing). Indirect nodes can handle inbound HTTP requests, but in that case if you're willing to share your IP address with the service that is trying to reach your node |
|
I strongly feel this is a non-issue because Hypergrid itself is opt-in. However, given the absolute state of Hypergrid GTM right now, I don't think it even matters if we try to rush an |
@Gohlub insisting I abstract my shit. smdh.
not necessary, EIP-3009 doesn't give much surface area for this kind of thing anyway
you know what it is
should still work with orthodox x402 clients (whatever that means) and also now x402scans more involved schema will let them generate a UI to make calls in-app
|
Ok, this is the last featureful commit. x402 + the extended schema for x402scan is done. To do:
|
|
upgrade path is fine. the whole state management thing in the app framework is a huge pain though, wish it would handle it more gracefully |
|
and the sh build.sh --env production path works just fine @Gohlub I want to get this out today. If you've like to break some of the business logic out into a separate file now's the time |
|
Let's make it so that |
moved logic to util.rs
|
"I approve" |

This Provider upgrade allows any provider to be queried via HTTP and for micropayments to settle via the x402 protocol.
It binds the
/xfourendpoint which accepts a GET with query parameters like/xfour?providername=name¶m1=value¶m2=value. These calls are then verified and settled with the facilitator.x402.rs endpoints (/verifyand/settle) before the parameters are passed to make the upstream call as usual.notes:
I have NOT yet tested this with Mainnet Base / USDC, only Sepolia. It should be fine, but EIP-3009 has been finicky so worth testing.
I have also not tested it on a provider that takes more than one parameter.
Also, this relies on features in branches of process lib and the macro that have not been merged yet, and are not necessarily going to be merged in the way that this PR implies.