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

PLT-7305 - Improve error message on invalid addresses #59

Merged
merged 9 commits into from
Dec 4, 2023
Merged

Conversation

palas
Copy link
Collaborator

@palas palas commented Nov 29, 2023

This PR:

  • Adds linting warning when addresses are invalid.
  • Show an error when trying to statically analyze contracts with invalid addresses (which solves the huge and cryptic error messages obtained before)
  • Updates the burn address in the escrow with collateral example, so that it has the right length and doesn't crash the static analysis
  • Adds pertinent tests

In order to achieve this, the PR also implements the address validation algorithm in PureScript, with the following caveats:

  • It doesn't support paying to script (it would be misleading since, as I understand it, it is not feasible because there is no way of providing a datum)
  • It doesn't support pointers (the serialization code in Haskell doesn't either)

I am tagging @bwbush for expertise on CIP19, and @hrajchert for expertise in PureScript and the Playground.

It would be good to double check the way I am validating the padding of the bech32, I am not 100% confident it is correct.

If you merge the PR, please squash it, because some of the intermediate commits are broken.

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Relevant tickets are mentioned in commit messages
    • Formatting, materialized Nix files, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@palas palas added the bug Something isn't working label Nov 29, 2023
@palas palas requested review from bwbush and hrajchert November 29, 2023 05:29
@palas palas self-assigned this Nov 29, 2023
Copy link
Collaborator

@bwbush bwbush left a comment

Choose a reason for hiding this comment

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

Looks great. It's nice to get this into the linter.

There's another test that could be done during linting: checking that the contract isn't mixing mainnet and testnet addresses, since there would be no network where the contract could run correctly. (In fact, we might want to have the "send to Runner" check that the contract has addresses that are consistent with the network.)

@palas
Copy link
Collaborator Author

palas commented Nov 29, 2023

There's another test that could be done during linting: checking that the contract isn't mixing mainnet and testnet addresses, since there would be no network where the contract could run correctly. (In fact, we might want to have the "send to Runner" check that the contract has addresses that are consistent with the network.)

Good idea, I made a ticket: PLT-8828

@palas palas merged commit 699b55a into main Dec 4, 2023
5 checks passed
@palas palas deleted the PLT-7305 branch December 4, 2023 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants