Skip to content
This repository was archived by the owner on Jan 20, 2026. It is now read-only.

Fixing Banksend unit tests #77

Merged
BrandonWeng merged 2 commits intomainfrom
bweng-fixunit-tests
Oct 28, 2022
Merged

Fixing Banksend unit tests #77
BrandonWeng merged 2 commits intomainfrom
bweng-fixunit-tests

Conversation

@BrandonWeng
Copy link
Contributor

@BrandonWeng BrandonWeng commented Oct 28, 2022

Describe your changes and provide context

With the deferred logic, we need to also check the pending deposits when subtracting coins from the module accounts, this was added to some of the methods, but some other areas were missed

Testing performed to validate your change

image

@BrandonWeng BrandonWeng marked this pull request as ready for review October 28, 2022 13:27
@BrandonWeng BrandonWeng changed the title Fix Unit test Fixing Banksend unit tests Oct 28, 2022
@BrandonWeng BrandonWeng requested a review from udpatil October 28, 2022 13:29

// Try subtract from the in mem var first, prevents the condition where
// the module may have a pending deposit that would be enough to pay for this send
ok := ctx.ContextMemCache().SafeSubDeferredSends(moduleName, amounts)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it safe to add this in the normal BurnCoins and not the DeferredBurn? Doesn't that make the deferred mem resources a potential race condition?

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 in the case where we call deferredMint and then call the regular burn, in this case if we don't check the deferred sends then it will error out with insufficient balance.

The deferred in mem variables are all blocked by mutex, I think in this case we only care about the absolute value of the in-mem variables at the end of FinalizeBlocker, as only a single value is written to the store

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha, makes sense

@BrandonWeng BrandonWeng merged commit 05cd6f9 into main Oct 28, 2022
@BrandonWeng BrandonWeng deleted the bweng-fixunit-tests branch October 28, 2022 14:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants