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

Address constructor input validation #48

Open
pszeder opened this issue Feb 6, 2022 · 4 comments
Open

Address constructor input validation #48

pszeder opened this issue Feb 6, 2022 · 4 comments

Comments

@pszeder
Copy link
Collaborator

pszeder commented Feb 6, 2022

When creating a new Address object, the public Address(string address) constructor only checks if the address is not null or whitespace. It seems that parsing and validation is implemented in Bech32.Decode(...) which is called within a try-catch block.

As a result, we can have Address objects that are invalid and do not represent any Cardano address. Furthermore, no exceptions are thrown when creating a transaction using such an invalid Address.

The generated CBOR will have null for the address field when used as an output. That is actually not a valid CBOR as per the specification and will likely result in an invalid CBOR/input exception when used with other software - Nami wallet in our case.

I would suggest that an Address should only be constructed if it is valid. That would be 'fail fast' and save people time and headaches.

@nothingalike
Copy link
Member

I put together a PR of something that might help. Take a look a let me know what you think

#49

@pszeder
Copy link
Collaborator Author

pszeder commented Feb 7, 2022

That is a good step and would allow us to check the addresses we use.

However, I think we should stop transactions from being created with invalid addresses if we don't stop invalid addresses from being instantiated.

@nothingalike
Copy link
Member

@pszeder thats a good point. will update PR

@nothingalike
Copy link
Member

@pszeder I closed out my PR. figured it got a bit stale and re-implementing a solution for this issue wouldn't take too much time. If you have thoughts around this, let me know. I can take another stab at a solution or implement some of your thoughts

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

No branches or pull requests

2 participants