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

test(store): fix tests after foundry update, move gas reports to separate tests #815

Merged
merged 1 commit into from
May 15, 2023

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented May 14, 2023

Fixes #814

  • Integrates the new expectEmit behavior introduced in feat(cheatcodes): Make expectEmit only work for the next call foundry-rs/foundry#4920 by calling functions on a wrapper contract (IStore(this)) instead of the library directly (StoreCore) to trigger a CALL.

  • Moves the gas reports to a separate test which still uses StoreCore, since calling the methods on a wrapper contract increases gas cost (but for the Store library gas reports we're interested in the library cost, not the contract call cost)

@alvrs alvrs requested a review from holic as a code owner May 14, 2023 09:36
@@ -80,21 +41,18 @@ contract StoreCoreTest is Test, StoreReadWithStubs {
vm.expectEmit(true, true, true, true);
emit StoreSetRecord(StoreCoreInternal.SCHEMA_TABLE, key, abi.encodePacked(schema.unwrap(), keySchema.unwrap()));

// !gasreport StoreCore: register schema
StoreCore.registerSchema(table, schema, keySchema);
IStore(this).registerSchema(table, schema, keySchema);
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have to cast to IStore here since this already extends the StoreMock contract, but I figured it makes it a bit more explicit to see what's going on here (and we don't care about gas cost in here because gas reports are moved to StoreCoreGas)

@alvrs alvrs merged commit e6e3cc2 into main May 15, 2023
@holic holic deleted the alvrs/fixforge branch June 23, 2023 11:33
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.

figure out failing emits with latest foundry
2 participants