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

Added test wasm writes with arbitrary key sizes #1253

Closed
wants to merge 2 commits into from

Conversation

SirTyson
Copy link
Contributor

What

Adds test wasm functions that write ContractData entries with arbitrary key sizes.

Why

This allows us to test network upgrades that reduce maxContractDataKeySizeBytes.

@SirTyson SirTyson requested a review from a team as a code owner November 27, 2023 23:11
@SirTyson SirTyson requested review from sisuresh and dmkozh November 27, 2023 23:12
@dmkozh
Copy link
Contributor

dmkozh commented Nov 27, 2023

This change is fine, but do we actually need it? This should be possible to cover by just passing the keys to the footprint, they don't have to be actually used.

@SirTyson
Copy link
Contributor Author

This change is fine, but do we actually need it? This should be possible to cover by just passing the keys to the footprint, they don't have to be actually used.

Sure, I guess it's not strictly required, but I think this is the easiest way. We don't allow writing keys above the limit, but we still allow reading. For the write test we can just put a dummy key in the footprint and make sure it fails, but for the read test we need some value with a large key that actually exists for us to read. Given our current testing framework, it seemed easiest to write this value via a contract call instead of manually injecting it into the ledger.

@dmkozh
Copy link
Contributor

dmkozh commented Nov 27, 2023

Given our current testing framework, it seemed easiest to write this value via a contract call instead of manually injecting it into the ledger.

Yeah, you're right, this probably will give us better coverage.

@dmkozh dmkozh enabled auto-merge November 27, 2023 23:55
@dmkozh dmkozh disabled auto-merge November 27, 2023 23:56
@SirTyson
Copy link
Contributor Author

Closing, after further discussion we're changing the behavior of ledger entry key size limit reductions and this test is no longer necessary.

@SirTyson SirTyson closed this Nov 27, 2023
@SirTyson SirTyson deleted the large-key-test branch November 28, 2023 00: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