-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix token gating #635
Fix token gating #635
Conversation
…pdate GroupHandlerTest accordingly
…tocol-contracts into fix-token-gating
… ExchangeHandlerTest.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good, a pointed out just some minor things.
67603dc
to
ff43c33
Compare
ff43c33
to
2b8f815
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just another small request.
…ndlerFacet.sol and update ExchangeHandlerTest.js to test for invalid token ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more requests.
8113c1a
to
16a05b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good... But I managed to find one more little thing (that's been there for ages actually, but seems a good time to fix it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
@@ -724,7 +763,7 @@ describe("IBosonExchangeHandler", function () { | |||
).to.revertedWith(RevertReasons.REGION_PAUSED); | |||
}); | |||
|
|||
it("buyer address is the zero address", async function () { | |||
it("buyer.address is the zero address", async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny change, but I can live with it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
* - EvaluationMethod.None and has fields different from 0 | ||
* - EvaluationMethod.Threshold and token address or maxCommits is zero | ||
* - EvaluationMethod.SpecificToken and token address or maxCommits is zero | ||
* A invalid condition is one that fits any of the following criteria: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/A invalid/An invalid/ ... one for the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed on #656 :)
* A invalid condition is one that fits any of the following criteria: | ||
* - EvaluationMethod.None: any field different from zero | ||
* - EvaluationMethod.Threshold: | ||
-Token address, maxCommits, or threshold is zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/-T/- T/ for later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed on #656 :)
Fix #627