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

fix: base token attributes #375

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

petarTxFusion
Copy link
Contributor

@petarTxFusion petarTxFusion commented Jan 17, 2025

What ❔

Import base token attributes from config

Why ❔

For custom chains base token attributes are read from contract which is incorrect. Contract always contains Ether attributes

Checklist
This PR fixes: #335

[+] 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.

@petarTxFusion petarTxFusion changed the title Fix/petar tx fusion fix base token attributes fix: base token attributes Jan 17, 2025
Copy link

github-actions bot commented Jan 17, 2025

API E2E Test Results

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

Results for commit 0273d84.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 17, 2025

Unit Test Results

    4 files    264 suites   11m 51s ⏱️
2 147 tests 2 146 ✅ 1 💤 0 ❌
2 361 runs  2 360 ✅ 1 💤 0 ❌

Results for commit 0273d84.

♻️ This comment has been updated with latest results.

Copy link

Visit the preview URL for this PR:
https://staging-scan-v2--pr-375-7oq1b03i.web.app

@@ -103,7 +106,21 @@ export class TransactionProcessor {
});
await Promise.all(
transactionData.tokens.map((token) => {
return this.tokenRepository.upsert(token);
if (token.l2Address.toLowerCase() === utils.L2_BASE_TOKEN_ADDRESS.toLowerCase()) {
this.tokenRepository.upsert({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please extend tests to cover this overwrite logic in the transaction.processor.spec

Copy link
Collaborator

Choose a reason for hiding this comment

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

@petarTxFusion this comment is still to be resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed this comment, its resolved now.

@petarTxFusion petarTxFusion force-pushed the fix/petarTxFusion-fix-base-token-attributes branch from e8d80ae to 0bda7fd Compare January 27, 2025 17:20
) {
this.logger = new Logger(TransactionProcessor.name);
console.log("Batch processing polling interval", configService);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this log statement was used for testing and should be removed.

@@ -101,9 +120,24 @@ export class TransactionProcessor {
blockNumber: blockNumber,
transactionHash: transactionData.transaction.hash,
});
console.log("Config l1 transaction processor", this.baseTokenConfig.l1Address);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto

await Promise.all(
transactionData.tokens.map((token) => {
return this.tokenRepository.upsert(token);
if (token.l2Address.toLowerCase() === utils.L2_BASE_TOKEN_ADDRESS.toLowerCase()) {
this.tokenRepository.upsert({
Copy link
Collaborator

Choose a reason for hiding this comment

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

The return statement is missing for this.tokenRepository.upsert so the Promise won't be returned from the map function and, as a result, the execution won't be properly awaited by Promise.all.

iconURL: this.baseTokenConfig.iconUrl,
});
} else {
this.tokenRepository.upsert(token);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto.

@vasyl-ivanchuk vasyl-ivanchuk force-pushed the fix/petarTxFusion-fix-base-token-attributes branch from 3c4e152 to 0273d84 Compare January 28, 2025 17:21
@vasyl-ivanchuk vasyl-ivanchuk merged commit 683e998 into main Jan 28, 2025
23 checks passed
@vasyl-ivanchuk vasyl-ivanchuk deleted the fix/petarTxFusion-fix-base-token-attributes branch January 28, 2025 17:37
Copy link

🎉 This PR is included in version 2.61.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base token attributes are incorrect after 800A contract update
3 participants