Feat/Delta -- depositNativeAndPreSign#223
Conversation
size-limit report 📦
|
…TokenModule ABI Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // returns whatever `contractCaller` returns | ||
| // to allow for better versatility |
There was a problem hiding this comment.
No it doesn't.
For signCancel it returns the signed message string.
For post it obviously returns POERST Response
|
|
||
| ## Composable Constructor Pattern | ||
|
|
||
| Every feature module exports a `constructXxx(options) => XxxFunctions` factory: |
There was a problem hiding this comment.
What prompt did you use to generate this?
Describe the repo and conventions or something?
There was a problem hiding this comment.
Why don't you ask Claud to fill in instructions for all other methods based on what it already wrote
Velenir
left a comment
Velenir
left a comment
There was a problem hiding this comment.
Would be great to add some tests.
Can be done last though
| > = async (signableOrderData, overrides = {}, requestParams) => { | ||
| // types allow to pass OrderData & extra_stuff, but tx will break like that | ||
| const typedDataOnly: SignableDeltaOrderData = { | ||
| ...signableOrderData, | ||
| data: sanitizeDeltaOrderData(signableOrderData.data), | ||
| }; | ||
|
|
||
| const orderHash = produceDeltaOrderHash(typedDataOnly); | ||
| const res = await depositNativeAndPreSign( | ||
| orderHash, | ||
| overrides, |
There was a problem hiding this comment.
It's very not obvious that deposit amount should be in overrides.value.
Better make it a separate input in the first param
| const { getDeltaContract } = constructGetDeltaContract(options); | ||
|
|
||
| const cancelAndWithdrawDeltaOrder: CancelAndWithdrawDeltaOrder<T> = async ( | ||
| { order, signature, bridgeOverride, cosignature, isFillable }, |
There was a problem hiding this comment.
In most cases neither we nor users will have bridgeOverride, cosignature, isFillable.
Make them optional and provide defaults
There was a problem hiding this comment.
Agree on both this comment and the comment above
Changed 4ac84b3
There was a problem hiding this comment.
I have a concern that it may be hard to cancel Orders with modified bridge (selectBridge option).
I don't think we can fetch that data from API and that forces users to keep track of what Order was created with what bridge
This may be a problem for BE to solve
There was a problem hiding this comment.
If we can count on API to return order.bridge stuff necessary, then we are still left without signature, unless it was saver on user side. It is never returned from API.
This makes it impossible to cancel ETH Orders unless they were made with 0x signature.
Currently that's how they will be made, exclusively. With depositAndPreSign.
But soon we'll want to reuse the available dETH balance to create ETH Orders with signTypedData only
|
|
||
| ## Composable Constructor Pattern | ||
|
|
||
| Every feature module exports a `constructXxx(options) => XxxFunctions` factory: |
There was a problem hiding this comment.
Why don't you ask Claud to fill in instructions for all other methods based on what it already wrote
…ositNativeAndPreSign
…erns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Velenir, I'll add tests after we agree on the core changes. Please review 🙏🏻 |
| order, | ||
| signature, | ||
| bridgeOverride = { | ||
| protocolData: DEFAULT_BRIDGE.protocolData, | ||
| protocolSelector: DEFAULT_BRIDGE.protocolSelector, |
There was a problem hiding this comment.
Why not
order,
signature,
bridgeOverride = {
protocolData: order.bridge.protocolData,
protocolSelector: order.bridge.protocolSelector,Why even have a separate bridgeOverride param?
Can order.bridge (if returned from API as part of DeltaOrderFromAPI) not match the bridgeOverride needed?
2 participants
@Velenir, wdyt about keeping
CLAUDE.mdfor further sessions with Claude?Note
Medium Risk
Adds new on-chain transaction methods (payable deposit + withdrawal/cancel flows) and wires them into the public SDK surface, so incorrect ABI/args/overrides could cause failed or unintended transactions.
Overview
Adds a new Delta on-chain
constructDeltaTokenModuleexposingcancelAndWithdrawDeltaOrder,withdrawDeltaNative, anddepositNativeAndPreSign(plus a helper that hashesSignableDeltaOrderDataand calls the deposit+presign flow withvalue).Wires the module into
constructAllDeltaOrdersHandlers,constructPartialSDKTxResponse inference, andsrc/index.tsexports; also movesDEFAULT_BRIDGEinto a dedicateddelta/constants.tsand bumps package version to9.3.4-dev.1(plus addsCLAUDE.mdarchitecture notes).Written by Cursor Bugbot for commit ac55dd9. This will update automatically on new commits. Configure here.