Add pack contract implementation #469
Conversation
|
This pull request introduces 1 alert when merging 652539b into ca4b84d - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging 4d41d0e into ca4b84d - view on LGTM.com new alerts:
|
src/contracts/pack.ts
Outdated
| private contractWrapper: ContractWrapper<PackContract>; | ||
| private storage: IStorage; | ||
| static schema = PackContractSchema; | ||
| // TODO: Change schema and deployment |
There was a problem hiding this comment.
Oops just forgot to remove the comment
| throw new Error( | ||
| `ERC20 token with contract address "${ | ||
| erc20.contractAddress | ||
| }" does not have enough allowance to transfer.\n\nYou can set allowance to the multiwrap contract to transfer these tokens by running:\n\nawait sdk.getToken("${ |
There was a problem hiding this comment.
lotthe conversion to Token contract struc and all the allowance checking logic could be shared between MW and pack no?
There was a problem hiding this comment.
There are some key differences because of the pack interface we want vs the one the contract implements, we need to do some multiplication/division in all these in the pack file that's different for each function in pack + different from multiwrap - which is why I opted to keep these all separate as verbose as it is, b/c handling all the casing seems messier and wanted to keep things explicit.
TL;DR, because contract uses total tokens + number of rewards (division), but we use tokens per reward + number of rewards (multiplication) we have to do some conversions between these interfaces which makes things different from MW and each other.
There was a problem hiding this comment.
i see - that makes sense.
| const ERC20RewardSchema = ERC20WrappableSchema.omit({ | ||
| quantity: true, | ||
| }).extend({ | ||
| quantityPerReward: PriceSchema, |
There was a problem hiding this comment.
wonder if it's really worth it to rename this. would help with code sharing if we didnt
There was a problem hiding this comment.
I think quantityPerReward is an important distinction here - added it because I originally had just quantity as the interface, but when I was using it, it felt like quantity was the number of tokens I was going to wrap (which it isn't), so it felt confusing. What I had in mind was wouldn't want to have a user put something like:
quantity: 1
totalRewards: 50
expecting to only lose 1 token, and then lose 50, so wanted to make it explicit.
| export const PackRewardsOutputSchema = z.object({ | ||
| erc20Rewards: z.array(ERC20RewardContentsSchema).default([]), | ||
| erc721Rewards: z.array(ERC721RewardContentsSchema).default([]), | ||
| erc1155Rewards: z.array(ERC1155RewardContentsSchema).default([]), |
There was a problem hiding this comment.
same for this - erc20Contents would make sense in both the pack and MW context right?
There was a problem hiding this comment.
Schema is a bit different for rewards vs MW because of the amounts stuff
| tokenId: "2", | ||
| amount: BigNumber.from(50), | ||
| quantityPerReward: 1, | ||
| totalRewards: 50, |
There was a problem hiding this comment.
this does read kinda nice though
test/pack.test.ts
Outdated
| tokenId: "2", | ||
| amount: BigNumber.from(50), | ||
| quantityPerReward: 1, | ||
| totalRewards: 50, | ||
| }, | ||
| ], | ||
| metadata: { |
There was a problem hiding this comment.
should we call this "packMetadata" to be clear its for the pack?
There was a problem hiding this comment.
yep good idea, maybe should to something similar for MW
src/contracts/pack.ts
Outdated
| * // The address of the contract that holds the rewards you want to include | ||
| * assetContract: "0x...", | ||
| * // The metadata of the pack | ||
| * const packMetadata = { |
There was a problem hiding this comment.
i would call this pack and metadata -> packMetadata
There was a problem hiding this comment.
fix the example after the metadata rename?
| * Create Pack | ||
| * @remarks Create a new pack with the given metadata and rewards and mint it to the connected wallet. | ||
| * | ||
| * @param metadataWithRewards - the metadata and rewards to include in the pack |
There was a problem hiding this comment.
let's add a See {@link Pack.createTo} in the remarks
src/contracts/pack.ts
Outdated
| * // The address of the contract that holds the rewards you want to include | ||
| * assetContract: "0x...", | ||
| * // The metadata of the pack | ||
| * const packMetadata = { |
There was a problem hiding this comment.
fix the example after the metadata rename?
No description provided.