Skip to content
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

bridge: transaction functionality #9

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

wildmolasses
Copy link
Collaborator

No description provided.

@wildmolasses
Copy link
Collaborator Author

wildmolasses commented Oct 10, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from 66bb0cb to 01db6e6 Compare October 12, 2023 12:58
@wildmolasses wildmolasses changed the base branch from 09-25-Bridge_page to main October 12, 2023 14:23
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from 01db6e6 to 50b6a86 Compare October 12, 2023 14:23
@netlify
Copy link

netlify bot commented Oct 12, 2023

Deploy Preview for warm-jelly-e126f7 ready!

Name Link
🔨 Latest commit c294a17
🔍 Latest deploy log https://app.netlify.com/sites/warm-jelly-e126f7/deploys/6537ed663f9aee000864831e
😎 Deploy Preview https://deploy-preview-9--warm-jelly-e126f7.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wildmolasses wildmolasses changed the base branch from main to 10-12-Change_networks_to_goerli_OP_goerli October 12, 2023 14:30
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from 50b6a86 to d0894a0 Compare October 12, 2023 14:30
@alexkeating alexkeating changed the base branch from 10-12-Change_networks_to_goerli_OP_goerli to main October 12, 2023 15:08
@alexkeating alexkeating mentioned this pull request Oct 12, 2023
@wildmolasses wildmolasses changed the base branch from main to 10-12-Nav_bar October 12, 2023 15:14
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from d0894a0 to edec8ff Compare October 12, 2023 15:14
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from edec8ff to 0a6e560 Compare October 13, 2023 10:09
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from 0a6e560 to 2626059 Compare October 13, 2023 10:11
@wildmolasses wildmolasses mentioned this pull request Oct 13, 2023
@alexkeating alexkeating changed the base branch from 10-12-Nav_bar to main October 16, 2023 16:58
@alexkeating alexkeating force-pushed the 10-10-bridge_transaction_functionality branch from 2626059 to 2249eb1 Compare October 16, 2023 16:58
@wildmolasses wildmolasses changed the base branch from main to 10-17-prefer_human-readable_ABI October 17, 2023 10:28
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch 2 times, most recently from 1886900 to 86c06c7 Compare October 17, 2023 10:39
@wildmolasses wildmolasses mentioned this pull request Oct 17, 2023
@alexkeating alexkeating changed the base branch from 10-17-prefer_human-readable_ABI to main October 17, 2023 17:28
@alexkeating alexkeating force-pushed the 10-10-bridge_transaction_functionality branch from 86c06c7 to 7f77d26 Compare October 18, 2023 13:07
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from 7f77d26 to 92808ae Compare October 19, 2023 10:08
@wildmolasses wildmolasses changed the base branch from main to 10-17-notifications October 19, 2023 10:08
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch 2 times, most recently from aa36681 to 2329c9b Compare October 19, 2023 11:39
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from 2329c9b to af3b62d Compare October 19, 2023 22:55
@alexkeating alexkeating changed the base branch from 10-17-notifications to main October 22, 2023 03:31
@wildmolasses wildmolasses changed the base branch from main to 10-17-notifications October 23, 2023 15:18
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from af3b62d to 1cf9072 Compare October 23, 2023 15:19
@wildmolasses wildmolasses marked this pull request as ready for review October 23, 2023 15:19
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch 2 times, most recently from 222c8ab to e637519 Compare October 23, 2023 20:40
Copy link
Collaborator

@alexkeating alexkeating left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Most things can be spun into another. Feel free to do so as you see fit.

chainId: l1Config.chain.id,
address: l1Config.tokenAddress,
abi: parseAbi([
"function allowance(address, address) public view returns (uint256)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to move these abis to a constants file in case we ever need to make changes

isSuccess: bridgeToL1IsSuccess,
// error: bridgeToL1Error,
write: bridgeToL1,
} = useContractWrite(bridgeToL1Config);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be good to abstract these to hooks

@@ -20,9 +21,12 @@ export const useEasyWrite = (
const error = prepareError || writeError;
const isLoading = prepareIsLoading || writeIsLoading;

useEffect(() => {
if (writeData?.hash) notify({ hash: writeData?.hash, status });
}, [notify, writeData?.hash, status]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be in a useEffect? Could it be an if statement outside of a useEffect?

)}
</div>
<div className="mt-5">
{bridgeTarget === BridgeTarget.L2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can eliminate one class of issues if we prompt the user to switch their chain. Either when they switch are try to bridge while on the wrong chain. I have an example of switching chains in the delegate page pr.

>
Bridge to {target.chain.name}
</button>
{bridgeTarget === BridgeTarget.L2 && needsAllowanceL1 ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this pr, but I noticed the amounts did not round nicely.

Screenshot from 2023-10-23 22-21-43

@@ -115,6 +204,7 @@ const Bridge = () => {
defaultValue=""
aria-invalid={isAmountError}
aria-describedby="amount"
onChange={handleInputChange}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This input can go negative

? "Transaction succeeded"
: "Transaction pending"}
</p>
<p className="mt-1 text-sm text-gray-500">
{description}
</p>
<p className="mt-1 text-sm text-gray-500">
<a href="https://etherscan.io">View on Etherscan</a>
<a href={`https://etherscan.io/tx/${hash}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be pulled out into a constant

data: bridgeToL1Response,
isLoading: bridgeToL1IsLoading,
isSuccess: bridgeToL1IsSuccess,
// error: bridgeToL1Error,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to delete and make an issue for these.

@wildmolasses
Copy link
Collaborator Author

wildmolasses commented Oct 24, 2023

Merge activity

@wildmolasses wildmolasses changed the base branch from 10-17-notifications to main October 24, 2023 16:13
@wildmolasses wildmolasses force-pushed the 10-10-bridge_transaction_functionality branch from e637519 to c294a17 Compare October 24, 2023 16:14
@wildmolasses wildmolasses merged commit 6dc610d into main Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants