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

feat: update token for upgradable transactions where implementation i… #348

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kiriyaga-txfusion
Copy link
Contributor

@kiriyaga-txfusion kiriyaga-txfusion commented Dec 4, 2024

…s token

What ❔

This PR updates the token reference in the database for Transparent and UUPS proxies linked to ERC20 token implementations. The upgradable transaction handler detects and replaces outdated tokens with the correct updated version.

Why ❔

Users can modify the symbol, name, and decimals of tokens deployed as proxies, so these details must be updated in the database to ensure they are displayed correctly in the UI. Note that Etherscan does not support this feature, and certain proxy patterns, such as Beacon proxies, are not affected in our case. (since beacon is upgraded but beacon proxy points to him).

fixes #326

Checklist

  • [+] PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • [+] Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.

Copy link

github-actions bot commented Dec 4, 2024

API E2E Test Results

207 tests   207 ✅  19s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit a7e1e8a.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 4, 2024

Unit Test Results

    4 files    260 suites   12m 19s ⏱️
2 100 tests 2 099 ✅ 1 💤 0 ❌
2 314 runs  2 313 ✅ 1 💤 0 ❌

Results for commit a7e1e8a.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Dec 4, 2024

Visit the preview URL for this PR:
https://staging-scan-v2--pr-348-rffzf1bu.web.app

"OwnershipTransferred(address,address)",
"AdminChanged(address,address)",
"OwnershipTransferred(address,address)",
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • OwnershipTransferred(address,address) is listed twice.
  • AdminChanged(address,address) doesn't contain indexed arguments, does it? If so, log.topics[1] will be always null for this event.
  • OwnershipTransferred(address,address) - where the first address is previousOwner and the second one is newOwner, why the previous owner (topics[1]) is used as the implementation address?

transactionHash: txReceipt.hash,
creatorAddress: txReceipt.from,
logIndex: log.index,
implementationAddress: address,
Copy link
Collaborator

Choose a reason for hiding this comment

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

After the latest changes were merged from main, the TS compiler complains that the isEvmLike field is missing for the ProxyAddress type. The exact error can be checked locally or in the failed GitHub Action for this PR.

transactionHash: "0x5e018d2a81dbd1ef80ff45171dd241cb10670dcb091e324401ff8f52293841b0",
address: "0x1BEB2aBb1678D8a25431d9728A425455f29d12B7",
topics: [
"0xbc7cd75a20ee27fd9adebab32041f755214dbc6bffa90cc0225b39da2e5c2d3b",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add test examples that test all event types that the handler processes. Currently only 0xbc7cd75a20ee27fd9adebab32041f755214dbc6bffa90cc0225b39da2e5c2d3b is covered. Ideally, I'd use real log examples.

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.

Token fields are not updated when token's implementation updates
3 participants