-
Notifications
You must be signed in to change notification settings - Fork 67
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
Adding tenderly simulation #49
base: main
Are you sure you want to change the base?
Conversation
@ragingrahul is attempting to deploy a commit to the Abacus Works Team on Vercel. A member of the Team first needs to authorize it. |
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.
Thanks @ragingrahul for the PR! This is a good start but it's incomplete and a bit sloppy. Is there a way to use the Tenderly APIs without a user's secret access key? If not, how did you intend to get that from the user?
package.json
Outdated
@@ -10,6 +10,7 @@ | |||
"@metamask/jazzicon": "https://github.com/jmrossy/jazzicon#7a8df28974b4e81129bfbe3cab76308b889032a6", | |||
"@rainbow-me/rainbowkit": "^0.11.0", | |||
"@tanstack/react-query": "^4.24.10", | |||
"axios": "^1.4.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.
Please use the browser native fetch
API, no need to add a new library just for a POST request :)
const [isOpen, setIsOpen] = useState(false); | ||
const disabled=false |
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 do you have a var here if it never changes?
<button onClick={async()=>{ | ||
handleClick() | ||
await simulateCall({contract,handleCalldata,chainId}) |
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.
Move this line to the click handler
if (!debugResult?.calldataDetails) return null; | ||
const { contract, handleCalldata } = debugResult.calldataDetails; | ||
function handleClick() { |
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.
event handlers (like click) should use lambda (arrow fn) syntax to be bound correctly.
const [isOpen, setIsOpen] = useState(false); | ||
const disabled=false | ||
const [buttonText,setButtonText]=useState<string>("Simulate Handle Call") |
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.
The <string>
type is inferred so it's unnecessary 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.
Change to Simulate call with Tenderly
save_if_fails: true, | ||
simulation_type: 'full', | ||
network_id: chainId, | ||
from: '0xdc6bdc37b2714ee601734cf55a05625c9e512461',//can be any address, doesn't matter |
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.
Use the 0 address if you just need a placeholder (ethers.constants.zeroAddress)
gas: 8000000, | ||
gas_price: 0, | ||
value: 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.
You'll need to use the real values from the message for an accurate simulation.
} | ||
); | ||
console.timeEnd('Simulation'); | ||
console.log(resp.data) |
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.
Use the logger
utility instead of console and add a message to provide context for the data you're logging
} | ||
) | ||
console.log(share) | ||
window.location.assign(`https://dashboard.tenderly.co/shared/simulation/${resp.data.simulation.id}`) |
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.
Please open the tenderly window in a new tab
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.
Good progress but please be more mindful to stay consistent with other parts of the codebase
setDisabled(false) | ||
setShowSpinner(false) |
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.
The disabled state always equals the showSpinner state so there's no reason to create 2 state vars. Can you please use the replace them with a single [loading, setLoading] state that is the inverse of disabled now?
from: '0x0000000000000000000000000000000000000000',//can be any address, doesn't matter | ||
to: contract, | ||
input:handleCalldata, | ||
gas: Number(message.totalGasAmount), |
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.
Please use BigNumber.from(message.totalGasAmount).toNumber()
to: contract, | ||
input:handleCalldata, | ||
gas: Number(message.totalGasAmount), | ||
gas_price: Number(message.totalPayment)/Number(message.totalGasAmount), |
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.
Use the computeAvgGasPrice util function here. It handles possible rounding issues:
https://github.com/hyperlane-xyz/hyperlane-explorer/blob/main/src/features/messages/cards/GasDetailsCard.tsx#L168
value: 0, | ||
} | ||
const resp=await fetch( | ||
`https://explorer.hyperlane.xyz/api/simulation`,{ |
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.
Remove the https://explorer.hyperlane.xyz
prefix. It's implied when coming from the same domain and will break the feature in dev builds
src/pages/api/simulation.ts
Outdated
const data=req.body | ||
|
||
const resp = await fetch( | ||
`https://api.tenderly.co/api/v1/account/${TENDERLY_USER}/project/${TENDERLY_PROJECT}/simulate`, |
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.
Check that these env vars are defined and throw a helpful here at the top if they're not
src/pages/api/simulation.ts
Outdated
}, | ||
} | ||
) | ||
res.json(simulationId) |
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.
Wrap the result in a success/failure result shape: https://github.com/hyperlane-xyz/hyperlane-explorer/blob/main/src/features/api/utils.ts
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.
Almost there!
|
||
|
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.
Spacing? Please run yarn prettier
and the files will get auto-formatted :)
|
||
} | ||
|
||
function computeAvgGasPrice(unit: string, gasAmount?: BigNumber.Value, payment?: BigNumber.Value) { |
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 did you duplicate this function here? Please de-dupe with the copy in GasDetailsCard and move it to messages/utils.ts
</div> | ||
</Modal> | ||
</> | ||
); | ||
} | ||
async function simulateCall({contract,handleCalldata,chainId,message}:{contract:string,handleCalldata:string,chainId:ChainId,message:Message}){ | ||
|
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.
Wrap the function contents here in a try/catch
and show an error message with toast.error
on failure
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.
Didn't wrap this in try/catch
, rather wrapped the backend call which returns failure call and checking if the call is success or failure 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.
Code looks okay, just one small comment, but there should be no yarn.lock file changes. That's likely why the CI is failing.
src/pages/api/simulation.ts
Outdated
) | ||
res.json(successResult(simulationId)) | ||
} catch (error) { | ||
res.json(failureResult("Could not simulate")) |
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.
Please change to "Error preparing Tenderly simluation"
export default async function handler(req, res) { | ||
const data = req.body; | ||
if (!TENDERLY_ACCESS_KEY || !TENDERLY_PROJECT || !TENDERLY_USER) { | ||
console.log('ENV not defined'); |
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.
Use logger instead of console, I believe this will raise a linter warning
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.
Change log message to something more descriptive: Tenderly key, project, or user not defined in env
const data = req.body; | ||
if (!TENDERLY_ACCESS_KEY || !TENDERLY_PROJECT || !TENDERLY_USER) { | ||
console.log('ENV not defined'); | ||
res.json(failureResult('Explorer Issues')); |
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.
Change to 'Tenderly credentials missing'
@@ -236,11 +258,51 @@ function CallDataModal({ debugResult }: { debugResult?: MessageDebugResult }) { | |||
</p> | |||
<LabelAndCodeBlock label="Recipient contract address:" value={contract} /> | |||
<LabelAndCodeBlock label="Handle function input calldata:" value={handleCalldata} /> | |||
<button onClick={handleClick} disabled={loading} className="underline text-blue-400"> |
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.
Show only when not loading (see feedback on discord).
Please also reduce text size: text-sm
Description
Implemented tenderly transaction simulation
To Know
TENDERLY_USER,TENDERLY_PROJECT,TENDERLY_ACCESS_KEY can be found USER & PROJECT and ACCESS-KEY
Related Issues
#2488