-
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
New tokenIds #583
New tokenIds #583
Conversation
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.
Besides what I put in the code, could you please add unit tests to validate that tokens created before and after EXCHANGE_ID_2_2_0 are burned correctly?
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.
forgot to choose "request changes"
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.
Tests for token burning before/after the upgrade are in #498 .
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 guess, a highlevel comment. The method we added : 'extractExchangeId' states that it is needed when a tokenID is passed in instead of an exchangeID...
when i look for the usage it only seems to be used in the exchangeHandlderFacet, is this meant to be the case ?
And given that the method, in the comments, states that it takes in a tokenID and outputs an exchangeID, i feel like the naming is misleading :
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: _exchangeId = extractExchangeId(_exchangeId);
protocol/facets/ExchangeHandlerFacet.sol: function extractExchangeId(uint256 _exchangeId) internal pure returns (uint256 exchangeId) {
Looking at the code, we don't seem to use these tokenIDs outside of Voucher and the Exchange handler, every other instance of tokenID seems to either be todo with token gating, token based auth, or twinning, which should be fine ...
Anyways
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.
@mischat Re: extractExchangeId
That was meant more for convenience, so users could pass in either tokenId
or exchangeId
.
But yeah, if we really wanted to support that, I would need to do it everywhere (there are methods in dispute and orchestration handler), and also change all natspec to say "exchangeId can either be exchangeId or tokenId`.
So after your comment I changed my mind and removed extractExchangeId
, since all protocol methods are clear that they expect exchangeId
. Anyways, whoever is interacting only with the protocol already operates only with the correct exchangeId.
But even if they have only tokenId, they can always easiliy convert it to exchangeId.
I'm running tests locally now and will push the changes soon.
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.
Thanks, this looks good to me
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
Replaces #582
Implements necessary changes to support
tokenId = <<uint128(offerId)>>.<<uint128(exchangeId)>>
Although initially meant only for preminted vouchers, it turned out it's easier to make the change for all new vouchers.
Main reason to do this is to remove binary search in
getPremintStatus
, which has now constant time and space complexity.Additinal implications:
_rangeOfferIds
was removed. Since binary search was removed, there is no need to require that preminted offer ranges are ordered.transferPremintedFrom
is not needed anymore since regular transfer functions are efficient enough