feat: Add validation for inputs currency and currencyToken.#683
feat: Add validation for inputs currency and currencyToken.#683yingyangxu2026 wants to merge 7 commits intomainfrom
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.
3 participants
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.