feat: Add validation for inputs currency and currencyToken. #683
Conversation
yingyangxu2026
requested review from
AndyBoWu,
DonFungible,
LeoHChen,
allenchuang,
bonnie57,
edisonz0718,
jacob-tucker,
limengformal and
lucas2brh
as code owners
yingyangxu2026
had a problem deploying
to
odyssey
GitHub Actions
Failure
yingyangxu2026
changed the title
…llowed function and remove the ticket ID from the unit test descriptions.
yingyangxu2026
had a problem deploying
to
odyssey
GitHub Actions
Failure
yingyangxu2026
left a comment
yingyangxu2026
left a comment
There was a problem hiding this comment.
I've made the changes. Could you please review them again?
yingyangxu2026
had a problem deploying
to
odyssey
GitHub Actions
Failure
| "0x1514000000000000000000000000000000000000" as Address, // WIP | ||
| ], | ||
| 1514: [ | ||
| "0x1514000000000000000000000000000000000000" as Address, // WIP |
There was a problem hiding this comment.
For WIP, could you use it from "./constants/common"?
And ERC20, could you import from the generated.ts
| * Throws an Error with the same message shape previously used by callers that | ||
| * validated arrays (e.g. group royalties distribution). | ||
| */ | ||
| export const assertCurrenciesAllowed = ( |
There was a problem hiding this comment.
Could you rename validateXX?
There was a problem hiding this comment.
And the logic has some duplication with the caller.
Could you clean it?
And I find the miss the validation, such as the claimAllRevenue method. Could you make sure every currency has been checked in the repo?
| }); | ||
| expect(result.txHash).equal(txHash); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I prefer you put the unit tests into the util file instead of the caller's unit test.
| import { WIP_TOKEN_ADDRESS } from "../../src/constants/common"; | ||
|
|
||
| // Use addresses that are whitelisted for unit tests that exercise currency checks. | ||
| export const mockERC20 = "0xF2104833d386a2734a4eB3B8ad6FC6812F29E38E"; |
There was a problem hiding this comment.
You should directly refer to the generated file.
|
|
||
| // Use addresses that are whitelisted for unit tests that exercise currency checks. | ||
| export const mockERC20 = "0xF2104833d386a2734a4eB3B8ad6FC6812F29E38E"; | ||
| export const mockAddress = WIP_TOKEN_ADDRESS; |
There was a problem hiding this comment.
Please remove mockAddress and directly use WIP_TOKEN_ADDRESS.
| * Throws an Error with a consistent message used across the SDK. | ||
| */ | ||
| export const assertCurrencyAllowed = (currency: Address, chainId: SupportedChainIds): void => { | ||
| if (currency === zeroAddress) return; |
There was a problem hiding this comment.
The lint will not pass when you run pnpm run lint. I think the reusable-build-test-workflow.yml fix step should not be corrected. We should use the pnpm run lint instead of pnpm fix to intercept the unmet rules.
Could you create a ticket to track it?
| const { royaltyPolicy, currency } = normalized; | ||
|
|
||
| // Validate currency whitelist for the resolved chain. | ||
| assertCurrencyAllowed(currency, resolvedChainId); |
There was a problem hiding this comment.
You should put the validation into the last step. It makes the entire logic correct.
Description
This pr is adding validation for inputs currency and currencyToken., includes：
Aeneid: Input parameterscurrency/currencyTokenacceptsWIPorMERC20.Mainnet: Input parameterscurrency/currencyTokenacceptsWIP.WIP address:
0x1514000000000000000000000000000000000000MERC20 address:
0xF2104833d386a2734a4eB3B8ad6FC6812F29E38ETest Plan
Add unit tests for the modified APIs.