Conversation
🟡 Heimdall Review Status
|
|
@jtang17 is attempting to deploy a commit to the Coinbase Team on Vercel. A member of the Team first needs to authorize it. |
b26c731 to
916aa6f
Compare
typescript/packages/mechanisms/aptos/src/exact/client/scheme.ts
Outdated
Show resolved
Hide resolved
typescript/packages/mechanisms/aptos/src/exact/server/register.ts
Outdated
Show resolved
Hide resolved
typescript/packages/mechanisms/aptos/src/exact/facilitator/scheme.ts
Outdated
Show resolved
Hide resolved
|
Cross-checking the x402/specs/schemes/exact/scheme_exact_aptos.md Lines 106 to 118 in 1884274
SVM has an additional security check to prevent the facilitator from signing away their own tokens, please review if this would also be needed in your case x402/typescript/packages/mechanisms/svm/src/exact/facilitator/scheme.ts Lines 202 to 211 in 1884274 |
|
It would be great if you could add some integration tests as well, see eg https://github.com/coinbase/x402/blob/main/typescript/packages/mechanisms/evm/test/integrations/exact-evm.test.ts |
|
Thanks a lot for the contribution @jtang17, this looks great! I highlighted a few places above, where I think the implementation could be even more aligned with the existing evm/svm mechanisms. The I would like to run some tests myself but I could't figure out how to get wAPT on testnet, could you please advice? Got at least some APT for the facilitator |
Thanks for the feedback. I've addressed the feedback and made the mechanism more consistent - I did also end up updating the v2 scheme for Aptos as part of this. You can get testnet APT either https://aptos.dev/network/faucet or through an account on https://geomi.dev (https://geomi.dev/manage/faucet) and testnet USDC here: https://faucet.circle.com/ |
2f38a3b to
15e1ebe
Compare
e2e/test.ts
Outdated
There was a problem hiding this comment.
Please add log( APTOS: ${networks.aptos.name} (${networks.aptos.caip2})); here
|
Thanks a lot for the quick update @jtang17, the changes look good and I ran the integration and e2e tests successfully! Concerning the integration tests could you please add aptos here: Concerning the e2e tests, could you please complete them by adding aptos to hono/next servers and axios client? |
| - **Sponsored Transactions**: Facilitators can pay gas fees on behalf of clients | ||
| - **Fungible Asset Transfers**: Uses Aptos's native `primary_fungible_store::transfer` | ||
| - **Network Support**: Mainnet (`aptos:1`) and Testnet (`aptos:2`) | ||
|
|
There was a problem hiding this comment.
Suggest to add link to faucets here
| const isPrimaryFungibleStore = | ||
| AccountAddress.ONE.equals(moduleAddress) && | ||
| moduleName === "primary_fungible_store" && | ||
| functionName === "transfer"; | ||
|
|
||
| const isFungibleAsset = | ||
| AccountAddress.ONE.equals(moduleAddress) && | ||
| moduleName === "fungible_asset" && | ||
| functionName === "transfer"; | ||
|
|
There was a problem hiding this comment.
Sorry I am not that familiar with aptos, could you elaborate a bit whats the difference between these functions and why the implementation picks one over the other?
There was a problem hiding this comment.
Added a note about when primary_fungible_store::transfer would be used vs fungible_asset::transfer
|
I see you made some changes @jtang17, please ping me when its ready to review again There seem to be some merge conflicts that still need to be resolved and changes in the go package that should be reverted |
e7312ee to
cdc7910
Compare
@phdargen - just rebased the dependency update and reverted go changes - PR should be ready for another review |
|
Hey @jtang17, the go changes are still there and the workflow tests fail |
e2e/clients/axios/index.ts
Outdated
| import { registerExactEvmScheme } from "@x402/evm/exact/client"; | ||
| import { registerExactSvmScheme } from "@x402/svm/exact/client"; | ||
| import { registerExactAptosScheme } from "@x402/aptos/exact/client"; | ||
| import { Account, Ed25519PrivateKey, PrivateKey, PrivateKeyVariants } from "@x402/aptos"; |
There was a problem hiding this comment.
Please fix import
| import { Account, Ed25519PrivateKey, PrivateKey, PrivateKeyVariants } from "@x402/aptos"; | |
| import { Account, Ed25519PrivateKey, PrivateKey, PrivateKeyVariants } from "@aptos-labs/ts-sdk"; |
and add "@aptos-labs/ts-sdk" to package.json
|
Please configure the |
|
Please make sure all integration tests pass, currently 3/9 fail |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hi @jtang17, could you please also add a publishing workflow for the new x402/aptos package following the evm reference here: |
e2e/servers/next/proxy.ts
Outdated
| "/api/protected-aptos-proxy": { | ||
| accepts: { | ||
| payTo: APTOS_PAYEE_ADDRESS || "0x0", | ||
| scheme: "exact", | ||
| price: "$0.001", | ||
| network: APTOS_NETWORK, | ||
| }, | ||
| extensions: { | ||
| ...declareDiscoveryExtension({ | ||
| output: { | ||
| example: { | ||
| message: "Protected endpoint accessed successfully", | ||
| timestamp: "2024-01-01T00:00:00Z", | ||
| }, | ||
| schema: { | ||
| properties: { | ||
| message: { type: "string" }, | ||
| timestamp: { type: "string" }, | ||
| }, | ||
| required: ["message", "timestamp"], | ||
| }, | ||
| }, | ||
| }), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Needs to be conditional here or e2e tests fail at runtime with RouteConfigurationError when no aptos env vars set. Same in express and hono
| "/api/protected-aptos-proxy": { | |
| accepts: { | |
| payTo: APTOS_PAYEE_ADDRESS || "0x0", | |
| scheme: "exact", | |
| price: "$0.001", | |
| network: APTOS_NETWORK, | |
| }, | |
| extensions: { | |
| ...declareDiscoveryExtension({ | |
| output: { | |
| example: { | |
| message: "Protected endpoint accessed successfully", | |
| timestamp: "2024-01-01T00:00:00Z", | |
| }, | |
| schema: { | |
| properties: { | |
| message: { type: "string" }, | |
| timestamp: { type: "string" }, | |
| }, | |
| required: ["message", "timestamp"], | |
| }, | |
| }, | |
| }), | |
| }, | |
| }, | |
| ...(APTOS_PAYEE_ADDRESS ? { | |
| "/api/protected-aptos-proxy": { | |
| accepts: { | |
| payTo: APTOS_PAYEE_ADDRESS, | |
| scheme: "exact", | |
| price: "$0.001", | |
| network: APTOS_NETWORK, | |
| }, | |
| extensions: { | |
| ...declareDiscoveryExtension({ | |
| output: { | |
| example: { | |
| message: "Protected endpoint accessed successfully", | |
| timestamp: "2024-01-01T00:00:00Z", | |
| }, | |
| schema: { | |
| properties: { | |
| message: { type: "string" }, | |
| timestamp: { type: "string" }, | |
| }, | |
| required: ["message", "timestamp"], | |
| }, | |
| }, | |
| }), | |
| }, | |
| }, | |
| } : {}), |
There was a problem hiding this comment.
Updated the hono, express, and next e2e to set these conditionally.
e2e/servers/next/proxy.ts
Outdated
| matcher: [ | ||
| "/api/protected-proxy", | ||
| "/api/protected-svm-proxy", | ||
| ...(APTOS_PAYEE_ADDRESS ? ["/api/protected-aptos-proxy"] : []), |
There was a problem hiding this comment.
This fails with
Next.js can't recognize the exported `config` field in route. `matcher` needs to be a static string or array of static strings or array of static objects.
120 | // Configure which paths the middleware should run on
121 | // Aptos path is only included if APTOS_PAYEE_ADDRESS is configured
> 122 | export const config = {
| ^^^^^^
123 | matcher: [
124 | "/api/protected-proxy",
125 | "/api/protected-svm-proxy",
I think here the route can be added unconditionally, please test
There was a problem hiding this comment.
Updated - it can be.
|
@phdargen workflow has been added for the aptos package - e2e tests seem to be passing though with some bazaar discovery issues when you do not run the tests with all options enabled (I think unrelated to these changes) |
|
@phdargen - are there any remaining changes needed for this PR? |
|
@erikreppel-cb @phdargen - resolved the conflicts! |
|
Hi @jtang17, my comments are all addressed and I successfully tested the integration + e2e tests last week. @CarsonRoscoe will test the vercel deployment of the testnet facilitator before we get this in, thanks for your patience 🙏 |
Description
Tests
Added tests for the aptos mechanisms package
For TypeScript: Run
cd typescript/packages/mechanisms/aptos && pnpm testChecklist