Skip to content
This repository has been archived by the owner on Jan 7, 2020. It is now read-only.

Add "areRightsConserved" and "isOfferSafe" functions with tests #123

Closed
wants to merge 1 commit into from

Conversation

katelynsills
Copy link
Contributor

This PR is part of the Zoe implementation. It adds helper functions for determining whether a reallocation of quantities conserves rights (has the same total quantity per issuer) and whether a reallocation is "offer-safe" for each user.

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

I like the way the tests are structured.

core/zoe/areRightsConserved.js Outdated Show resolved Hide resolved
core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
core/zoe/isOfferSafe.js Show resolved Hide resolved
core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
test/unitTests/core/zoe/test-isOfferSafe.js Outdated Show resolved Hide resolved
core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
test/unitTests/core/zoe/test-utils.js Show resolved Hide resolved
@katelynsills
Copy link
Contributor Author

Whoops, Mark reminded me that it will likely be haveAtMost rather than haveAtLeast. I'll need to make that change

@katelynsills
Copy link
Contributor Author

Whoops, Mark reminded me that it will likely be haveAtMost rather than haveAtLeast. I'll need to make that change

Done

core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
const refundOk = rulesPerIssuer
.map((rule, i) => {
insist(
allowedRules.includes(rule.rule),
Copy link
Member

Choose a reason for hiding this comment

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

When there are more than two issuers, for many players, they will only submit rules for a subset of the issuers. While this representation can deal with that, it seems unnatural. We should simply allow the rules array for a given player to have a null or something for the issuers that it doesn't care about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea. Should we support null and undefined as offerDescElems or only null? In general, are there benefits to preferring one over the other? I'm allowing both for now.

core/zoe/isOfferSafe.js Outdated Show resolved Hide resolved
core/zoe/utils.js Outdated Show resolved Hide resolved
const anyTrue = (prev, curr) => prev || curr;

// https://stackoverflow.com/questions/17428587/transposing-a-2d-array-in-javascript/41772644#41772644
const transpose = matrix =>
Copy link
Member

Choose a reason for hiding this comment

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

Do you actually need both representations, or could you just keep it in the transposed form?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only need the transposed version to do the conservation of rights check, because that's summing by issuer column, whereas most everything else is concerned with a player perspective (the rows). Since we don't actually store both, I think this is ok. Did you think this would give bad performance?

core/zoe/utils.js Show resolved Hide resolved
core/zoe/utils.js Outdated Show resolved Hide resolved
@katelynsills
Copy link
Contributor Author

I'm closing this PR in favor of a new set of Zoe PRs in which this one (#123), #125, and #136 are squashed and then pulled apart again.

This was referenced Oct 4, 2019
@katelynsills katelynsills deleted the offer-safety-helpers branch October 5, 2019 03:49
@katelynsills katelynsills mentioned this pull request Dec 4, 2019
5 tasks
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.

3 participants