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

Update Bug_0xblank #398

Merged
merged 16 commits into from
Dec 28, 2024
Merged

Conversation

KevinMB0220
Copy link
Contributor

@KevinMB0220 KevinMB0220 commented Dec 18, 2024

Address Input Validation Fix

Fixes #345

Types of change

  • Bug

Comments (optional)

Fixed a bug where the AddressInput allowed invalid inputs such as 0x.
Now the input is validated to ensure it meets the correct format:

  • It must start with 0x.
  • It must contain only hexadecimal characters.

If validation fails, an error message is displayed: "Invalid address format".

Comment on lines 63 to 67
const isValidHexAddress = (value: string): boolean => {
const hexAddressRegex = /^0x[0-9a-fA-F]+$/;
return hexAddressRegex.test(value);
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets put it in the utils file (i think we have this util already). we don't want duplicates of the same function since they would be a pain to maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was given another recommendation that 0x be accepted as felt252 and that I should add a 0 to 0x to be accepted

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep, aligned on that end

Copy link
Collaborator

Choose a reason for hiding this comment

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

please make sure 0x0x0 does not work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll check that it aligns with that

Copy link
Collaborator

@metalboyrick metalboyrick left a comment

Choose a reason for hiding this comment

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

seems invalid address still accepted, should not be accepted

expected behavior:

  • 0x treated as 0x0
  • 0x0x0 should be error
  • x0 should be error
  • 0x<something> should be correct
image

@KevinMB0220
Copy link
Contributor Author

seems invalid address still accepted, should not be accepted

expected behavior:

  • 0x treated as 0x0
  • 0x0x0 should be error
  • x0 should be error
  • 0x<something> should be correct
image

The validation is now ready so that what you requested is not allowed and only valid addresses are allowed.

Copy link
Collaborator

@Nadai2010 Nadai2010 left a comment

Choose a reason for hiding this comment

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

Hi @KevinMB0220 this is still malfunctioning, can check please? It doesn't do this for me.

Captura desde 2024-12-23 07-27-06
Captura desde 2024-12-23 07-26-22
Captura desde 2024-12-23 07-26-43
Captura desde 2024-12-23 07-26-55

@KevinMB0220
Copy link
Contributor Author

Hi @KevinMB0220 this is still malfunctioning, can check please? It doesn't do this for me.

Captura desde 2024-12-23 07-27-06 Captura desde 2024-12-23 07-26-22 Captura desde 2024-12-23 07-26-43 Captura desde 2024-12-23 07-26-55

Ok i check

Copy link
Collaborator

@metalboyrick metalboyrick left a comment

Choose a reason for hiding this comment

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

seems like this is not the spot to fix. might want to take a look at the InputBase and Input component to solve this issue.

@metalboyrick
Copy link
Collaborator

@KevinMB0220 what we want to do is to check for the input, not the address display

@KevinMB0220
Copy link
Contributor Author

@KevinMB0220 what we want to do is to check for the input, not the address display

Can I change something in the faucet since the input address is declared there, so the validation is more direct and functional there?

@KevinMB0220 KevinMB0220 force-pushed the feat/Bug_0xblank branch 2 times, most recently from a5a4ca3 to fe3b7bf Compare December 26, 2024 00:12
@metalboyrick
Copy link
Collaborator

@KevinMB0220 what we want to do is to check for the input, not the address display

Can I change something in the faucet since the input address is declared there, so the validation is more direct and functional there?

yep sure!

Copy link
Collaborator

@metalboyrick metalboyrick left a comment

Choose a reason for hiding this comment

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

sorry for the long winded review but we need to get this right. Asides from the AddressInput component you would also need to validate the one on the Debug Page (which doesn't use this component)

@metalboyrick
Copy link
Collaborator

sorry for the long winded review but we need to get this right. Asides from the AddressInput component you would also need to validate the one on the Debug Page (which doesn't use this component)

i think you can fix the AddressInput component first, let's think about how to abstract the validations to the inputs on the debug page later on (cc @Nadai2010 )

Copy link
Collaborator

@metalboyrick metalboyrick 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 to me!

@metalboyrick metalboyrick merged commit f46ced0 into Scaffold-Stark:main Dec 28, 2024
1 check passed
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.

[BUG] 0x<blank> input considered valid on some fields.
3 participants