Revealables #231
Revealables #231
Conversation
| * @return revealedAssetIds The assetIds to reveal | ||
| * @return assetToReplaceIds The assetIds to replace | ||
| */ | ||
| function getRevealedAssets( |
There was a problem hiding this comment.
The name of the function sounds like the assets were already revealed, but comment says assetIds to reveal like they are yet to be revealed
There was a problem hiding this comment.
I would rename it to getRevealableAssets and revealableAssetIds if this is about assets that are yet to be revealed
There was a problem hiding this comment.
It's tricky.. since by calling this method you are effectively revealing them. What about getAssetsToReveal and assetsToRevealIds?
There was a problem hiding this comment.
Yes I like it more, but then we have an opposite issue, that once they are revealed, the name will not be very accurate, as they are already revealed, but var is still called toReveal. Another alternative is assetIdsOnReveal or something like that. But if you don't like any of these options, you can leave as is
There was a problem hiding this comment.
Since this became reveal instead, I think revealedAssetsIds is now fine
test/extensions/revealable.ts
Outdated
|
|
||
| // --------------- TESTS ----------------------- | ||
|
|
||
| describe('RMRKTokenPropertiesMock', async function () { |
There was a problem hiding this comment.
Would be nice to add test with replace id, also with contract where there's more than 1 revealable asset
There was a problem hiding this comment.
Actually, the tests already uses replace id. Also, I'm focusing on revealable logic which is the abstract contract I'm implementing.
Revealer here is just a mock to prove the concept, in the future we may add a few implementations and only then I would add more specific cases.
There was a problem hiding this comment.
Sure, but tests and mocks are like a documentation in this case, so good to have it to cover many cases and be descriptive
Description
Adds Revealable/Revealer extensions with abstract impl for the first.
Checklist
PR-Codex overview
Detailed summary