-
Notifications
You must be signed in to change notification settings - Fork 266
Update withdrawal-flow.mdx #682
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
Conversation
fix: documentation error
WalkthroughThe update involves a significant change in the SDK's Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Additional Context UsedLanguageTool (13)
Path-based Instructions (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
✅ Deploy Preview for docs-optimism ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
pages/stack/protocol/withdrawal-flow.mdx (3)
Line range hint
29-31
: Consider revising the list format to ensure consistency and clarity.- * `_target`, target address on L1. - * `_message`, the L1 transaction's calldata, formatted as per the [ABI](https://docs.soliditylang.org/en/v0.8.19/abi-spec.html) of the target address. - * `_minGasLimit`, The minimum amount of gas that the withdrawal finalizing transaction can provide to the withdrawal transaction. + * `_target`: Target address on L1. + * `_message`: The L1 transaction's calldata, formatted as per the [ABI](https://docs.soliditylang.org/en/v0.8.19/abi-spec.html) of the target address. + * `_minGasLimit`: The minimum amount of gas that the withdrawal finalizing transaction can provide to the withdrawal transaction.
Line range hint
31-31
: Consider rephrasing to avoid wordiness.- In order to account for the gas consumed in the `L1CrossDomainMessenger.relayMessage` function's execution, extra gas will be added on top of the `_minGasLimit` value by the `CrossDomainMessenger.baseGas` function when `sendMessage` is called on L2. + To account for the gas consumed in the `L1CrossDomainMessenger.relayMessage` function's execution, extra gas will be added to the `_minGasLimit` value by the `CrossDomainMessenger.baseGas` function when `sendMessage` is called on L2.
Line range hint
55-55
: Add a comma after 'Typically' for better readability.- Typically this is done [by the SDK](https://sdk.optimism.io/classes/crosschainmessenger#proveMessage-2). + Typically, this is done [by the SDK](https://sdk.optimism.io/classes/crosschainmessenger#proveMessage-2).
@smartcontracts What do i need to do to get this merged? Even though it is small, contributing to OP would be a cool thing for me |
commit d7fd352 Merge: 44b27dd 458ddc8 Author: cpengilly <[email protected]> Date: Tue May 14 13:03:11 2024 -0700 Merge pull request #682 from jtfirek/patch-1 Update withdrawal-flow.mdx commit 458ddc8 Author: Jacob T Firek <[email protected]> Date: Fri May 10 14:56:10 2024 -0500 Update withdrawal-flow.mdx fix: documentation error
Description
I believe that the transaction hash that is passed into the
proveMessage()
is the corresponding L2 transaction. I don't think there is any L1 transactions corresponding to this withdraw yet.