-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: local testing framework and swapAndBridge example #9
Conversation
|
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 general looks great, mostly nits, but also some need to look at what happens with surplus for bridging (this likely may require some use of weiroll), as well as giving guidance for determining an appropriate gasLimit
.
examples/swapAndBridge.ts
Outdated
target: USDC, | ||
callData: fnCalldata( | ||
'approve(address,uint256)', | ||
ABI_CODER.encode(['address', 'uint'], [GNOSIS_CHAIN_BRIDGE, buyAmount]) |
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.
nit: note that as it is a sell
order, there may be at least buyAmount
, but there may actually be more than buyAmount
...
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.
Additionally, please make consistent use of specifying the bitwidth of uint
(i.e. no uint
, use uint256
🙏 )
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, was a conscious choice, surplus will just stay in the user controlled proxy
examples/swapAndBridge.ts
Outdated
); | ||
const hooks = { | ||
post: [ | ||
{ target: factory, callData: hooksCalldata, gasLimit: '1000000000' }, |
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 think the example should methodology for determining a sane gasLimit
(note that given the use of ecrecover
within the signature verification, the gas used isn't fixed)
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 provider.estimateGas
but its gonna fail for the posthooks usually since those tokens wont exist on the proxy at that point.
394b7d9
to
a9330df
Compare
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.
Approved on the basis that comments are addressed
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.
Extensive documentation required on the WeiRoll
.
}; | ||
|
||
// encode the input part of the weiroll command | ||
export const encodeInput = ( |
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 would be great to understand what all these are (without specific reference to weiroll docs)
examples/swapAndBridge.ts
Outdated
encodeInput( | ||
encodeInputArg(true, 0), |
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.
nit: describe what this is doing. Write it long hand in comments. Developers should be able to reference this example and have minimum additional work required to understand what is happening so they can then apply it to their own use case.
END_OF_ARGS, | ||
END_OF_ARGS, | ||
END_OF_ARGS, | ||
END_OF_ARGS, | ||
END_OF_ARGS |
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.
Why this? a simple comment would help
END_OF_ARGS, | ||
END_OF_ARGS | ||
), | ||
encodeInputArg(true, 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.
What is this?
END_OF_ARGS | ||
), | ||
encodeInputArg(true, 1), | ||
USDC |
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.
Assuming this is the target, document as such
END_OF_ARGS, | ||
END_OF_ARGS | ||
), | ||
END_OF_ARGS, |
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.
Why is this different to the previous? Assuming this is because the output doesn't have to be put somewhere, but document it as such
examples/swapAndBridge.ts
Outdated
const state = [ | ||
hexZeroPad(proxyAddress, 32), // address to query the balance of | ||
'0x', // this is where balance output will be written | ||
hexZeroPad(USDC, 32), // USDC token address | ||
]; |
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.
Explain in more detail what is happening here. It's not clear why the balanceOf
is in the previous weiroll commands, but then there is state here.
Introduces common utilities for anvil setup, contracts deployment, funding, creating and settling orders, etc. And an example to swap and bridge using these utilties.