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.

Sign up for GitHub

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

Jump to bottom

prevent duplicate tokens being added in adminJS #1852

Merged
merged 1 commit into from
Oct 2, 2024
Merged

prevent duplicate tokens being added in adminJS #1852

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions src/server/adminJs/tabs/tokenTab.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,43 @@ export const createToken = async (
organizations,
} = request.payload;
try {
if (!address || !decimals || !name || !networkId || !symbol) {
message = 'Please fill all required fields';
type = 'danger';
return {
notice: {
message,
type,
},
};
}
Comment on lines +132 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate 'decimals' is a valid number

Currently, 'decimals' is only checked for a truthy value. If 'decimals' cannot be converted to a valid number, Number(decimals) will result in NaN, which may cause issues when saving the token. Consider adding a validation check to ensure 'decimals' is a valid number before proceeding.

Apply this diff to improve validation:

        if (!address || !decimals || !name || !networkId || !symbol) {
          message = 'Please fill all required fields';
          type = 'danger';
          return {
            notice: {
              message,
              type,
            },
          };
        }
+       if (isNaN(Number(decimals))) {
+         message = 'Decimals must be a valid number';
+         type = 'danger';
+         return {
+           notice: {
+             message,
+             type,
+           },
+         };
+       }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!address || !decimals || !name || !networkId || !symbol) {
message = 'Please fill all required fields';
type = 'danger';
return {
notice: {
message,
type,
},
};
}
if (!address || !decimals || !name || !networkId || !symbol) {
message = 'Please fill all required fields';
type = 'danger';
return {
notice: {
message,
type,
},
};
}
if (isNaN(Number(decimals))) {
message = 'Decimals must be a valid number';
type = 'danger';
return {
notice: {
message,
type,
},
};
}

const duplicateAddress = await Token.createQueryBuilder('token')
.where('LOWER(token.address) = LOWER(:address)', { address })
.andWhere('token.networkId = :networkId', {
networkId: Number(networkId),
})
.getOne();

const duplicateSymbol = await Token.createQueryBuilder('token')
.where('LOWER(token.symbol) = LOWER(:symbol)', { symbol })
.andWhere('token.networkId = :networkId', {
networkId: Number(networkId),
})
.getOne();

if (duplicateSymbol || duplicateAddress) {
message = `Token ${
duplicateAddress ? 'address' : 'symbol'
} already exists!`;
type = 'danger';
return {
record: {},
notice: {
message,
type,
},
};
}
Comment on lines +156 to +168
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve error message to reflect all duplicate fields

When both the token address and symbol already exist, the current error message only indicates one of them. To provide clearer feedback to the user, consider updating the error message to list all duplicate fields that are causing the conflict.

Apply this diff to enhance the error message:

        if (duplicateSymbol || duplicateAddress) {
-          message = `Token ${
-            duplicateAddress ? 'address' : 'symbol'
-          } already exists!`;
+          let duplicateFields = [];
+          if (duplicateAddress) duplicateFields.push('address');
+          if (duplicateSymbol) duplicateFields.push('symbol');
+          const fields = duplicateFields.join(' and ');
+          message = `Token ${fields} already exist${duplicateFields.length > 1 ? '' : 's'}!`;
          type = 'danger';
          return {
            record: {},
            notice: {
              message,
              type,
            },
          };
        }

This change ensures that if both the address and symbol are duplicates, the error message will inform the user accordingly.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (duplicateSymbol || duplicateAddress) {
message = `Token ${
duplicateAddress ? 'address' : 'symbol'
} already exists!`;
type = 'danger';
return {
record: {},
notice: {
message,
type,
},
};
}
if (duplicateSymbol || duplicateAddress) {
let duplicateFields = [];
if (duplicateAddress) duplicateFields.push('address');
if (duplicateSymbol) duplicateFields.push('symbol');
const fields = duplicateFields.join(' and ');
message = `Token ${fields} already exist${duplicateFields.length > 1 ? '' : 's'}!`;
type = 'danger';
return {
record: {},
notice: {
message,
type,
},
};
}

newToken = Token.create({
name,
symbol,
Expand Down
Loading