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

Improve Transaction API validation #371

Merged
5 commits merged into from
Jun 13, 2022
Merged

Improve Transaction API validation #371

5 commits merged into from
Jun 13, 2022

Conversation

prix-uniris
Copy link
Contributor

Description

Additional Checks on Structure Changesets to validate proper lengths for following

  • Ownerships
    • ownership recipients
  • UCOLedger transfers
  • NFTLedger transfers
  • recipients in a transaction
  • Code Length (Fetched from the config)

Fixes #330

Type of change

  • New feature (non-breaking change which adds functionality)
    Forces API to precheck the sizes which are supported by transaction encoding before serializing

How Has This Been Tested?

  • test "should return an error if the code length is more than 5 MB"
  • test "should return an error if the uco ledger transfers are more than 256"
  • test "should return an error if the nft ledger transfers are more than 256"
  • test "should return an error if the recipients are more that 256"
  • test "should return an error ownerships are more than 256."
  • test "should return an error authorized keys in a ownership can't be more than 256"

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@prix-uniris prix-uniris requested a review from a user June 9, 2022 08:27
ghost
ghost reviewed Jun 9, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Good first PR 🎉
I pointed some comments to improve it

lib/archethic_web/controllers/api/schema/transaction_data.ex Outdated
|> cast_embed(:ownerships)
|> validate_length(:code,
max: @code_max_size,
message: "code size can't be more than " <> Integer.to_string(@code_max_size) <> " bytes"
Copy link

Choose a reason for hiding this comment

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

If you want you can use string interpolation:

"code size can't be more than #{Integer.to_string(@code_max_size)} bytes"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 😅😅

lib/archethic_web/controllers/api/schema/transaction_data.ex Outdated
max: @code_max_size,
message: "code size can't be more than " <> Integer.to_string(@code_max_size) <> " bytes"
)
|> validate_length(:ownerships, max: 256, message: "ownerships can not be more that 256")
|> validate_content_size()
Copy link

Choose a reason for hiding this comment

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

I think you can remove it and use validate_length instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay sure

lib/archethic_web/controllers/api/schema/transaction_data.ex Outdated
# Validate data size here
# Calculate max. content size here based on args
# Custom validators in ecto
# validate_length // works with lists, strings
Copy link

Choose a reason for hiding this comment

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

Little miss here 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup will clean this also

config/config.exs Outdated
@@ -57,6 +57,9 @@ config :archethic, :marker, "-=%=-=%=-=%=-"
# size represents in bytes binary
config :archethic, :transaction_data_content_max_size, 3_145_728

# size represents in bytes binary
config :archethic, :transaction_data_code_max_size, 5_242_880
Copy link

Choose a reason for hiding this comment

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

Why ~42KB ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5242880 Bytes = 5 MB. I think that is good enough for the code size and can be changed in config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to 24Kb in Config

@ghost ghost changed the title #330 Improve Transaction API validation Jun 9, 2022
@ghost ghost added feature New feature request API Involve API facing user labels Jun 9, 2022
@prix-uniris prix-uniris requested a review from a user June 9, 2022 10:05
@ghost ghost added the good first issue Good for newcomers label Jun 9, 2022
@ghost ghost merged commit 4c7b023 into archethic-foundation:develop Jun 13, 2022
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
Assignees
No one assigned
Labels
API Involve API facing user feature New feature request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@prix-uniris