-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add paymaster API SNIP #132
Conversation
SNIPS/snip_x.md
Outdated
From the point of view of the user, the critical juncture during a "full" interaction with a paymaster (i.e. one that involves calling method `buildTypedData` and then `execute`) is the inspection of the typed data returned to them by the `buildTypedData` endpoint. In case the user account is already deployed, the array of calls in this object SHOULD either | ||
1. Be exactly equal to the array of calls submitted by the user. | ||
2. The first call is a single `Transfer` call originating from the user account and calling the ERC-20 contract of the requested token, followed by the array of calls submitted by the user. |
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.
this can be slightly different is we use SNIP-9 revision 3
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.
Added clarification in dcbcd07
assets/snip-x/paymaster_api.json
Outdated
], | ||
"result": { | ||
"name": "result", | ||
"description": "The hash of the transaction broadcasted by the paymaster", |
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.
not sure if out of scope. But what happens if there's a gas spike and the transaction is stuck. The paymaster will need to replace it with a new transaction to avoid having a stuck relayer. In that case there will be a new tx hash...
Maybe there could be another function to get the latest tx?
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.
Added in dcbcd07
assets/snip-x/paymaster_api.json
Outdated
} | ||
}, | ||
{ | ||
"name": "paymaster_trackingIdToLatestHash", |
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'm thinking we might need to return a status as well. If the transaction is subsidized the paymaster can just update the tx hash (if they support this functionality). But when paying in a different token the paymaster can't just increase the gas price, the user needs to sign the data again to agree to the new fee. I think the paymaster should be able to indicate this situation here
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.
In the flow where the user first asks the typed data, and the paymaster returns the typed data with a max fee (possibly with transfer call preprended), then I imagine that this max fee should have some leeway to allow for bumps in gas/tip - the redeposited amount should also account for this. Also, once a tx is sitting in the mempool and not rebumped, it's hard to say if it's still valid or it should be regarded as "failed" from the POV of the user, because it's failing to enter a block. Not sure that this logic should belong to the paymaster.
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 see your point, but if the relayer tx is stuck. The paymaster will eventually replace the transaction with another transaction with the same (regular) nonce, but carrying a different outside execution. At this point the paymaster knows for sure that the tx is stuck forever
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 get your point now. The status is actually a signal from the paymaster saying "i'm not dealing with this execution request anymore". Theoretically with nonce channels (or even without, just by dropping the relayer), the paymaster is not forced to make the tx permanently invalid by sending one with the same nonce, I agree it's good to signal that the paymaster is not trying anymore
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.
Done in 74fbb7a
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.
Reviewable status: 0 of 2 files reviewed, 14 unresolved discussions (waiting on @bbrandtom and @sgc-code)
assets/snip-x/paymaster_api.json
line 157 at r3 (raw file):
"type": "object", "properties": { "max_amount": {
Rename to estimated_amount? this is confusing with the actual tx field IMO
assets/snip-x/paymaster_api.json
line 281 at r3 (raw file):
}, { "$ref": "#/components/errors/INVALID_REQUEST_PAYLOAD"
What is invalid in this context? In general the RPC errors should only be semantic, the structure is enforced by the specs themselves
assets/snip-x/paymaster_api.json
line 284 at r3 (raw file):
}, { "$ref": "#/components/errors/INVALID_SIGNATURE"
The paymaster service is expected to run a starknet_call
to is_valid_signature
with the message hash? If yes, can we make it explicit in the spec somewhere?
assets/snip-x/paymaster_api.json
line 293 at r3 (raw file):
}, { "$ref": "#/components/errors/TRANSACTION_EXECUTION_ERROR"
Is this specific to the execution of validate? If yes, let's rename to something like VALIDATE_EXECUTION_ERROR/VALIDATION_FAILED (generalizes invalid signature). Also, possible duplicated with INVALID_SIGNATURE
assets/snip-x/paymaster_api.json
line 351 at r3 (raw file):
}, "required": [ "transaction_hash",
What should the transaction hash be in active/dropped?
assets/snip-x/paymaster_api.json
line 425 at r3 (raw file):
"$ref": "#/components/schemas/FELT", "description": "The transaction ID", "title": "Transaction hash"
Are the title and description fields wrong? IIUC this is meant for the paymaster backend to associate the request with a fresh tx hash, rather than be an actual Starknet tx hash
assets/snip-x/paymaster_api.json
line 450 at r3 (raw file):
"types": { "type": "object", "additionalProperties": {
should be properties? same for the other typed data versions
assets/snip-x/paymaster_api.json
line 457 at r3 (raw file):
} }, "primaryType": {
Is it important to be consistent with SNIP12 here or can we keep snake case property names? if the latter, this is relevant to all the other versions as well
assets/snip-x/paymaster_api.json
line 847 at r3 (raw file):
}, { "title": "No fee",
How about a string enum with one value NO_FEE? I don't think spaces are allowed in property names
assets/snip-x/paymaster_api.json
line 861 at r3 (raw file):
"Fee Amount": { "title": "Fee amount", "$ref": "#/components/schemas/u256_AS_FELTS"
Why does the spec need to know about Cairo's representation of uint26? (part of the broader discussion on should the paymaster spec use SNIP12 terminology)
assets/snip-x/paymaster_api.json
line 987 at r3 (raw file):
"type": "integer", "enum": [ 0,
Why should we support Cairo0 here? They would in any case have to go through a dedicated flow to upgrade
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.
Reviewable status: 0 of 2 files reviewed, 12 unresolved discussions (waiting on @ArielElp, @bbrandtom, and @sgc-code)
assets/snip-x/paymaster_api.json
line 281 at r3 (raw file):
Previously, ArielElp wrote…
What is invalid in this context? In general the RPC errors should only be semantic, the structure is enforced by the specs themselves
Removed
assets/snip-x/paymaster_api.json
line 284 at r3 (raw file):
Previously, ArielElp wrote…
The paymaster service is expected to run a
starknet_call
tois_valid_signature
with the message hash? If yes, can we make it explicit in the spec somewhere?
See reply to TRANSACTION_EXECUTION_ERROR comment below
assets/snip-x/paymaster_api.json
line 293 at r3 (raw file):
Previously, ArielElp wrote…
Is this specific to the execution of validate? If yes, let's rename to something like VALIDATE_EXECUTION_ERROR/VALIDATION_FAILED (generalizes invalid signature). Also, possible duplicated with INVALID_SIGNATURE
The paymaster will simulate the tx before sending it, and this error corresponds to a reverted tx. The INVALID_SIGNATURE
error should be a "sub-error" which corresponds to a revert specifically due to the invalid user signature (which happens in execute so I wouldn't call it VALIDATE_EXECUTION_ERROR/VALIDATION_FAILED). Maybe should rename TRANSACTION_EXECUTION_ERROR to SIMULATED_TRANSACTION_REVERTED?
assets/snip-x/paymaster_api.json
line 351 at r3 (raw file):
Previously, ArielElp wrote…
What should the transaction hash be in active/dropped?
The hash of the most recent tx sent by the paymaster and corresponding to the ID
assets/snip-x/paymaster_api.json
line 425 at r3 (raw file):
Previously, ArielElp wrote…
Are the title and description fields wrong? IIUC this is meant for the paymaster backend to associate the request with a fresh tx hash, rather than be an actual Starknet tx hash
Done
assets/snip-x/paymaster_api.json
line 450 at r3 (raw file):
Previously, ArielElp wrote…
should be properties? same for the other typed data versions
Copied from the corresponding objects in the wallet <> Dapp RPC
assets/snip-x/paymaster_api.json
line 987 at r3 (raw file):
Previously, ArielElp wrote…
Why should we support Cairo0 here? They would in any case have to go through a dedicated flow to upgrade
Done
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.
Reviewable status: 0 of 2 files reviewed, 10 unresolved discussions (waiting on @bbrandtom, @leo-starkware, and @sgc-code)
assets/snip-x/paymaster_api.json
line 351 at r3 (raw file):
Previously, leo-starkware wrote…
The hash of the most recent tx sent by the paymaster and corresponding to the ID
What if it was never sent, should it be optional? Also you should add what you wrote here to the description
assets/snip-x/paymaster_api.json
line 450 at r3 (raw file):
Previously, leo-starkware wrote…
Copied from the corresponding objects in the wallet <> Dapp RPC
Can you change to type array and items, it's also "wrong" there IMO>
assets/snip-x/paymaster_api.json
line 987 at r3 (raw file):
Previously, leo-starkware wrote…
Done
Can we remove the entire field then, and assume that calldata is supposed to be encoded for Cairo1 accounts?
assets/snip-x/paymaster_api.json
line 1015 at r4 (raw file):
"message": "An error occurred (CLASS_HASH_NOT_SUPPORTED)" }, "TRANSACTION_EXECUTION_ERROR": {
This was changed in the latest RPC to have a recursive structure, can you take it from the v0.8.0 branch?
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.
Reviewable status: 0 of 2 files reviewed, 10 unresolved discussions (waiting on @ArielElp, @bbrandtom, and @sgc-code)
assets/snip-x/paymaster_api.json
line 351 at r3 (raw file):
Previously, ArielElp wrote…
What if it was never sent, should it be optional? Also you should add what you wrote here to the description
A tracking ID comes into existence after a successful response from the execute
endpoint, which is also required to return a tx hash (= the first tx hash to be associated with the tracking ID). Will add more details to the description
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.
Reviewable status: 0 of 2 files reviewed, 9 unresolved discussions (waiting on @ArielElp, @bbrandtom, and @sgc-code)
assets/snip-x/paymaster_api.json
line 987 at r3 (raw file):
Previously, ArielElp wrote…
Can we remove the entire field then, and assume that calldata is supposed to be encoded for Cairo1 accounts?
IMO it's weird to have all the fields there but have the version be inferred implicitly by the consumer of the spec
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.
Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @ArielElp, @bbrandtom, and @sgc-code)
assets/snip-x/paymaster_api.json
line 1015 at r4 (raw file):
Previously, ArielElp wrote…
This was changed in the latest RPC to have a recursive structure, can you take it from the v0.8.0 branch?
Done.
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.
Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @bbrandtom, @leo-starkware, and @sgc-code)
assets/snip-x/paymaster_api.json
line 450 at r3 (raw file):
Previously, ArielElp wrote…
Can you change to type array and items, it's also "wrong" there IMO>
Reverting, we need unknown property names here
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.
Reviewable status: 0 of 2 files reviewed, 7 unresolved discussions (waiting on @ArielElp, @bbrandtom, and @sgc-code)
assets/snip-x/paymaster_api.json
line 457 at r3 (raw file):
Previously, ArielElp wrote…
Is it important to be consistent with SNIP12 here or can we keep snake case property names? if the latter, this is relevant to all the other versions as well
I think the coupling with SNIP 12 and SNIP 9 makes life easier for integrations. Keeping for now
assets/snip-x/paymaster_api.json
line 847 at r3 (raw file):
Previously, ArielElp wrote…
How about a string enum with one value NO_FEE? I don't think spaces are allowed in property names
This is for consistency with proposed changes to SNIP 9
assets/snip-x/paymaster_api.json
line 861 at r3 (raw file):
Previously, ArielElp wrote…
Why does the spec need to know about Cairo's representation of uint26? (part of the broader discussion on should the paymaster spec use SNIP12 terminology)
Same discussion as above re SNIP 12
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.
Reviewed 1 of 2 files at r2, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @bbrandtom and @sgc-code)
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bbrandtom and @sgc-code)
This change is