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

fix: Resolve errors in pre-commit hooks (ruff, mypy) #43

Closed
wants to merge 9 commits into from

Conversation

talhahussain7
Copy link
Contributor

No description provided.

@talhahussain7 talhahussain7 changed the base branch from main to dev June 25, 2024 18:08
@talhahussain7 talhahussain7 force-pushed the feat/fix-pre-commit-checks#15 branch from acbfb57 to e639ab2 Compare July 3, 2024 15:17
Copy link
Collaborator

@theeldermillenial theeldermillenial left a comment

Choose a reason for hiding this comment

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

It looks the reason for the errors are from the addition of the classmethod decorators.

Also, while looking through the code, I found a few concerning issues.

I made a couple of comments, but I think it might be a good idea to really section this off and open a multiple PRs for no more than 3-5 files at a time. Otherwise, I will be concerned that this large of a PR is going to cause some things to slip through that shouldn't.

I want to make sure we aren't going to have any regressions, and the current state of this PR will definitely cause at least one regression.

Comment on lines +140 to +149
try:
address = Address.decode(addr)
except (DecodingException, TypeError):
error_msg = "Failed to decode "
try:
if address is None:
address = Address(VerificationKeyHash(bytes.fromhex(addr)))
except ValueError as err:
error_msg += f"and construct by key Hash: {addr}"
raise ValueError(error_msg) from err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like over-engineering. Line 143 in particular will be a problem because no error is raised even though the error message is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is related to the error that I mentioned in our last meeting :)

Comment on lines -297 to -328
def get_datum_from_address(address: Address) -> ScriptReference:
SCRIPT_SELECTOR = """
SELECT ENCODE(tx.hash, 'hex') as "tx_hash",
tx_out.index as "tx_index",
tx_out.address,
ENCODE(datum.hash,'hex') as "datum_hash",
ENCODE(datum.bytes,'hex') as "datum_cbor",
COALESCE (
json_build_object('lovelace',tx_out.value::TEXT)::jsonb || (
SELECT json_agg(
json_build_object(
CONCAT(encode(ma.policy, 'hex'), encode(ma.name, 'hex')),
mto.quantity::TEXT
)
)
FROM ma_tx_out mto
JOIN multi_asset ma ON (mto.ident = ma.id)
WHERE mto.tx_out_id = tx_out.id
)::jsonb,
jsonb_build_array(json_build_object('lovelace',tx_out.value::TEXT)::jsonb)
) AS "assets",
ENCODE(s.bytes, 'hex') as "script"
FROM tx_out
LEFT JOIN tx ON tx.id = tx_out.tx_id
LEFT JOIN datum ON tx_out.inline_datum_id = datum.id
LEFT JOIN block on block.id = tx.block_id
LEFT JOIN script s ON s.id = tx_out.reference_script_id
WHERE tx_out.payment_cred = %(address)b
AND tx_out.inline_datum_id IS NOT NULL
ORDER BY block.time DESC
LIMIT 1
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be getting deleted, which is not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

@talhahussain7
Copy link
Contributor Author

talhahussain7 commented Jul 11, 2024

It looks the reason for the errors are from the addition of the classmethod decorators.

Also, while looking through the code, I found a few concerning issues.

I made a couple of comments, but I think it might be a good idea to really section this off and open a multiple PRs for no more than 3-5 files at a time. Otherwise, I will be concerned that this large of a PR is going to cause some things to slip through that shouldn't.

I want to make sure we aren't going to have any regressions, and the current state of this PR will definitely cause at least one regression.

Probably a good idea to do it in smaller bits instead.

Ruff: Found 703 errors
mypy: Found 249 errors

The changes have been all of the places...

It'll be nice to know if any additional configurations are required to run the tests on the dev branch to begin with.

@talhahussain7 talhahussain7 deleted the feat/fix-pre-commit-checks#15 branch September 6, 2024 22:05
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.

2 participants