Skip to content

Conversation

@BowTiedWoo
Copy link
Contributor

This PR adds test coverage for the contract-call? arguments representations in memory. These tests help ensure that function arguments are stored and handled correctly, particularly for memory allocation and representation consistency.

Related PR: stacks-network/stacks-core#6009
Related Issue: #633

@BowTiedWoo BowTiedWoo requested a review from Acaccia April 30, 2025 15:08
Copy link
Contributor

@Acaccia Acaccia left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand how those tests "ensure that function arguments are stored and handled correctly, particularly for memory allocation and representation consistency."

I understand that these tests are there to check the fix from stacks-network/stacks-core#6009, but here the fix is not applied yet. So if those tests don't fail with the current code, what are they supposed to show?

@BowTiedWoo
Copy link
Contributor Author

Thank you for the feedback!

The reason the tests are not failing is due to the current implementation over-allocating space for argument representations.

These tests remain valid, as they are intended to verify the new implementation, which should allocate only the exact amount of space required.

@BowTiedWoo BowTiedWoo requested a review from Acaccia May 1, 2025 13:05
@Acaccia
Copy link
Contributor

Acaccia commented May 8, 2025

I'm not super convinced here...
Could you please describe the testing process once the fix is applied? What should happen if it works? What should fail if it doesn't? For which reasons? How to debug? (etc...)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

4 participants