Skip to content

Conversation

@stefanoamorelli
Copy link
Contributor

@stefanoamorelli stefanoamorelli commented May 15, 2025

Tip

Better reviewed commit-by-commit. This PR follows conventional, atomic commits.

In this PR we introduce the Java implementation + tests (>85% test coverage), following the existing typescript, and go implementations.

A follow-up PR will include a more extensive Java Spring example.


Closes: #164

@vercel
Copy link

vercel bot commented May 15, 2025

Someone is attempting to deploy a commit to the Coinbase Team on Vercel.

A member of the Team first needs to authorize it.

@cb-heimdall
Copy link

cb-heimdall commented May 15, 2025

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@erikreppel-cb
Copy link
Contributor

@stefanoamorelli amazingly quick turn around, will review. We need you to sign your commits before we merge https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@stefanoamorelli stefanoamorelli force-pushed the feat/java branch 4 times, most recently from c79fa24 to eb86718 Compare May 16, 2025 10:05
@stefanoamorelli
Copy link
Contributor Author

@stefanoamorelli amazingly quick turn around, will review. We need you to sign your commits before we merge https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

Authored, signed the commits, and rebased ✅

@stefanoamorelli stefanoamorelli force-pushed the feat/java branch 3 times, most recently from 0ac9bef to 010bcec Compare May 25, 2025 19:54
@erikreppel-cb
Copy link
Contributor

@stefanoamorelli apologies for the delay, CB has been on recharge week. I'm not very versed in Java so gonna have someone from our team who is take a look, should merge this week.

Your commits still aren't showing up as signed, do need that or I physically can't merge :)

@stefanoamorelli
Copy link
Contributor Author

@erikreppel-cb no worries, thanks for letting me know!

I've force-pushed the signed commits, does it show on your end too now?

image

And sure, looking forward to your feedback!

@erikreppel-cb
Copy link
Contributor

@stefanoamorelli yes now they're signed, thank you!

<suppress checks="VisibilityModifier" files=".*[/\\]client[/\\].*\.java"/>

<!-- Suppress indentation checks on all files -->
<suppress checks="Indentation" files=".*\.java"/>
Copy link

Choose a reason for hiding this comment

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

Why do we suppress this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need actually, removed.

<module name="IllegalInstantiation"/>
<module name="SimplifyBooleanExpression"/>
<module name="SimplifyBooleanReturn"/>

Copy link

Choose a reason for hiding this comment

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

Consider adding a Javadoc checker to ensure documentation, e.g.:

Suggested change
<module name="JavadocPackage"/> <module name="MissingJavadocType"> <property name="scope" value="public"/> <property name="excludeScope" value="package"/> </module>
<module name="MissingJavadocMethod"> <property name="scope" value="public"/>
<property name="excludeScope" value="package"/>
<property name="allowMissingPropertyJavadoc" value="true"/> </module>
<module name="MissingJavadocVariable"> <property name="scope" value="public"/>
</module>
<module name="JavadocStyle"> <property name="checkFirstSentence" value="true"/>
<property name="endOfSentenceFormat" value="([.?!][ \t\n\r\f<])|([.?!]$)"/>
<property name="checkEmptyJavadoc" value="true"/>
<property name="checkHtml" value="true"/>
</module>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

Json.MAPPER.writeValueAsString(body)))
.build();

String json = http.send(request, HttpResponse.BodyHandlers.ofString()).body();
Copy link

Choose a reason for hiding this comment

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

Calling .body() directly means we don't do any error handling - feels like we should catch some common cases? CC @erikreppel-cb

* Implement with web3j, Solana-J, etc., depending on your payment scheme.
*/
public interface CryptoSigner {
/** Returns a hex-encoded signature covering the given payload map. */
Copy link

Choose a reason for hiding this comment

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

Could we expand on this Javadoc? It's a bit too abstract to reasonably implement, since interpretations will vary depending on protocol (Ethereum, Solana, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a more descriptive Javadoc, please let me know if we want more details or a different structure.


/** Contract for calling an x402 facilitator (HTTP, gRPC, mock, etc.). */
public interface FacilitatorClient {
VerificationResponse verify(String paymentHeader,
Copy link

Choose a reason for hiding this comment

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

We should add Javadoc to all public methods


/** Identifies a payment scheme+network pair that a facilitator supports. */
public class Kind {
public String scheme; // e.g. "exact"
Copy link

Choose a reason for hiding this comment

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

All of these should be public final since they are read only

private final Map<String, BigInteger> priceTable; // path → amount
private final FacilitatorClient facilitator;

public PaymentFilter(String payTo,
Copy link

Choose a reason for hiding this comment

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

Javadoc these params. priceTable especially needs a detailed description since it dictates the behavior

Server->>Facilitator: Settle payment (async)
```

## Error Handling
Copy link

Choose a reason for hiding this comment

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

We don't do any error handling for non-402 cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated ✔️

/* -------- best-effort async settlement (errors ignored) -------- */
try {
facilitator.settle(header, buildRequirements(path));
} catch (Exception ignored) { }
Copy link

Choose a reason for hiding this comment

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

We don't do any error handling for non-402 cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Impoved error handling - most importantly, removed the best-effort settlement to align with the typescript and go counterparties and indeed handle settlement failures.

@stefanoamorelli stefanoamorelli force-pushed the feat/java branch 4 times, most recently from 2af4568 to a9b495f Compare May 27, 2025 20:43
@erikreppel-cb
Copy link
Contributor

@stefanoamorelli looks great, your recent commits aren't showing up as signed, if you can sign those I'll merge

Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
Signed-off-by: Stefano Amorelli <stefano@amorelli.tech>
@stefanoamorelli
Copy link
Contributor Author

stefanoamorelli commented May 28, 2025

@erikreppel-cb good stuff, thanks! 🎉 Commits should be signed now, sorry about that ✔️ (I think when re-basing from Github UI they lose their signature)

@stefanoamorelli
Copy link
Contributor Author

@erikreppel-cb just checking in, should I rebase or is there anything needed from my side before merging?

@erikreppel-cb erikreppel-cb merged commit add0b93 into coinbase:main Jun 2, 2025
10 of 11 checks passed
@erikreppel-cb
Copy link
Contributor

@stefanoamorelli merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Add Java Implementation

4 participants