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

Native balance refactoring #87

Merged
merged 6 commits into from
Sep 15, 2020
Merged

Native balance refactoring #87

merged 6 commits into from
Sep 15, 2020

Conversation

maurolacy
Copy link
Contributor

Refactor cw1-subkeys native Balance into cw0.

Use it in the cw20-atomic-swap contract Balance.

Closes the native part of #76.

@maurolacy maurolacy requested a review from ethanfrey September 12, 2020 16:47
@maurolacy maurolacy self-assigned this Sep 12, 2020
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks quite good. I just left some comments on naming and some usage details to make this a bit clearer. Logic looks good. Except maybe the is_empty

Also, I would rebase on master since #86 was merged.

contracts/cw20-atomic-swap/src/balance.rs Show resolved Hide resolved
contracts/cw20-atomic-swap/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw20-atomic-swap/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw20-atomic-swap/src/balance.rs Show resolved Hide resolved
@maurolacy maurolacy force-pushed the native-balance-refactoring branch from c877128 to f25477e Compare September 14, 2020 16:27
@maurolacy maurolacy force-pushed the native-balance-refactoring branch from 7e2373d to c9d315e Compare September 14, 2020 17:06
@maurolacy
Copy link
Contributor Author

maurolacy commented Sep 14, 2020

I like how it is now. Normalizing is a relatively costly (and mutable) operation, and in this way we don't enforce it on every is_empty() call.

Also, with your proposal for checking for empty balances using an iterator, is_empty is now independent of normalization.

Finally, I moved that is_empty() check to the NativeBalance struct directly.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good, I would just remove the normalize and merge

packages/cw0/src/balance.rs Show resolved Hide resolved
contracts/cw20-atomic-swap/src/contract.rs Outdated Show resolved Hide resolved
@maurolacy
Copy link
Contributor Author

maurolacy commented Sep 15, 2020

With the change to is_empty below, you can remove this line, right?

You can, in the sense that the is_empty() check will still work. That is, it doesn't depend on a previous normalization anymore.

But, keeping it does no harm, as normalization is a good thing, i. e. it's better to store a normalized balance that an un-normalized one.

I just didn't want to couple the normalization with the empty check, because of, you know, purity. :-)

Suppose in the future we notice that normalizing is an expensive operation, by example when you have a large list of coins. We can add a check / modification to the main contract code, to account for that. That is: now we can change normalization, and normalization usage in a per-contract basis, without affecting is_empty() at all.

@ethanfrey ethanfrey merged commit 322f192 into master Sep 15, 2020
@ethanfrey
Copy link
Member

But, keeping it does no harm, as normalization is a good thing, i. e. it's better to store a normalized balance that an un-normalized one.

This is a good point and if we did other manipulations on the array (like with escrow), I would add it.

For now, I would remove it and save the gas costs. And we use that where we want to do more array manipulations (and maybe optimize all the NativeBalance ops to work with normalize balances)

@ethanfrey ethanfrey deleted the native-balance-refactoring branch September 15, 2020 10:51
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