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

Added "To:" and "From:" along with checks of EOA to EOA or contract interaction #17

Closed
wants to merge 3 commits into from

Conversation

PoulavBhowmick03
Copy link

Addresses #14
Closes #14

This is how the tx/[hash].tsx looks like now, after the changes
image

Changes made

  1. Added a useState to check if the "to" address is a contract or not
  2. The component conditionally renders "Contract Interaction" or "EOA Transfer" based on the state.
  3. Both from and to addresses are rendered as links to their respective account detail pages.

This is how I have implemented the changes to this. Let me know if this is good enough or there has to be any other changes made

Copy link
Collaborator

@saimeunt saimeunt left a comment

Choose a reason for hiding this comment

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

@PoulavBhowmick03 almost there, please move your client-side logic to server-side using my requested changes comment to get this PR merged.

const TransactionDetails = ({ transaction }: { transaction: Transaction }) => {
const [isContract, setIsContract] = useState<boolean | null>(null);

useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

While the logic is good, we don't want it to happen client-side because we want to mirror how the explorer will work when the accompanying indexer is ready.
Move your logic server-side by adding an optional isContractInteraction property in the Transaction type declared in @/lib/types.
Populate this computed property using your client-side logic in the top-level Page component and pass the updated transaction object to this TransactionDetails component.
Do not display "Contract interaction" or "EOA transfer" next to the to address, instead modify the label to display "Interacted With (To):" in case of contract interaction.

@PoulavBhowmick03
Copy link
Author

Okay! Will make these changes

@PoulavBhowmick03
Copy link
Author

@saimeunt Can you check the changes that I made, moved the isContractInteraction to lib/types.ts

Copy link
Collaborator

@saimeunt saimeunt left a comment

Choose a reason for hiding this comment

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

@PoulavBhowmick03 I don't get it, you completely removed the contract detection logic on the to address which was correct, it just needed to be put one level up in the root component, which is a React Server Component so we can compute the isContractInteraction prop server-side in one go.

Please reintroduce the correct behavior.

@@ -10,6 +10,7 @@ export type Transaction = {
value: bigint;
gasPrice?: bigint;
timestamp: bigint;
isContractInteraction?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can remove this, we'll stick with the transaction interface provided by JSON-RPC which doesn't involve an isContractInteraction property.

</CardContent>
</Card>
);
const TransactionDetails = ({ transaction }: { transaction: Transaction }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a top-level isContractInteraction prop here on the TransactionDetails component that will be computed one level up in the root component: https://github.com/walnuthq/op-scan/blob/main/src/components/pages/tx/index.tsx#L6-L7

Compute the property and pass it to this component along with the transaction.
Replicate the logic you wrote before by calling getCode on the to address of the tx.

@saimeunt
Copy link
Collaborator

@PoulavBhowmick03 Hey do you plan to finish this before the end of the hack?
Shouldn't be too hard to get this done quickly.

@saimeunt saimeunt closed this Jul 2, 2024
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.

From/To in tx details page
2 participants