Dropper #53
Dropper #53
Conversation
…ng in a constructor
…ted amount of tokens
| } | ||
| } | ||
|
|
||
| contract ERC1155CompatibleClaimProxy is ClaimProxy { |
There was a problem hiding this comment.
question: ClaimProxy is always ERC721 or ERC1155? Is there a scenario in which it needs to be both?
There was a problem hiding this comment.
This is just for tests @karacurt .
| LibDiamond.enforceIsContractOwner(); | ||
|
|
||
| // Set up server side signing parameters for EIP712 | ||
| LibSignatures._setEIP712Parameters("Game7 Dropper", "0.3.0"); |
There was a problem hiding this comment.
suggestion (non-blocking): Shouldn't those values be received in init as parameters? That way, we can reuse this dropper code with other customers.
There was a problem hiding this comment.
This is how the dropper from Moonstream was set. The Naming has no prevention. I see the EIP712Parameters in this case as the stamp of creation. Giving another location in the contract that says the contract is from Game7.
There was a problem hiding this comment.
@karacurt : I prefer to do it this way - keeps things simple and I have been using versions to key storage migrations on other diamonds so I'd prefer to have a bit more control on these.
We do have users who have made customizations to DropperV2 - they simply wrote their own init function.
|
|
||
| DroppableToken memory tokenMetadata; | ||
| tokenMetadata.tokenType = tokenType; | ||
| tokenMetadata.tokenAddress = tokenAddress; |
There was a problem hiding this comment.
issue (blocking): Shouldn't we enforce a tokenAddress in case of a native token? maybe address(0)?
There was a problem hiding this comment.
We can add in a statement requiring it set to address(0)
There was a problem hiding this comment.
Doesn't matter to me either way. It is a nice user protection, as long as you put a useful error message @mrk-hub .
| return LibDropper.dropperStorage().DropRequestClaimed[dropId][requestId]; | ||
| } | ||
|
|
||
| function withdrawERC20(address tokenAddress, uint256 amount) public onlyTerminusAdmin { |
There was a problem hiding this comment.
suggestion (non-blocking): Would it be good to add the recipient parameter in case we are withdrawing to another account? I don't know if we will have this use case, but that can be useful.
There was a problem hiding this comment.
No we do not have the ability for someone else to claim a drop for someone else, or for you to send your drop to someone else. This would need to be discussed with the original devs on this part. I would suggest against it just incase of a frontend hack where the hackers just send everything to a hardcoded address. This happened on quickswap(uniswap clone) on matic a few years ago. The hackers just sent the tokens to be received to their address.
There was a problem hiding this comment.
This is not a claim - this is an admin function used to manage funds on the contract. I like @karacurt 's suggestion, but I also don't think it's a high priority.
There was a problem hiding this comment.
The easiest way to take funds off of the contract as an admin is to sign a message authorizing yourself to claim those tokens. In that sense, I don't even think the withdraw* functionality is necessary.
There was a problem hiding this comment.
Should we remove all the withdraw functions then?
| //todo: Check if "game7dao.eth" wanted or something else | ||
| bytes32 constant DROPPERV3_STORAGE_POSITION = keccak256("game7dao.eth.storage.Dropper"); |
There was a problem hiding this comment.
todo: Just leaving this comment to help you remember if this needs to be changed before merge
There was a problem hiding this comment.
Need to ask Neeraj if this is what should be here or not.
There was a problem hiding this comment.
What did we decide upon?
There was a problem hiding this comment.
I believe it was remove the .eth portion will push that change
| @@ -0,0 +1,41 @@ | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
suggestion:
Please double-check in other places to change our licenses to MIT
| // SPDX-License-Identifier: Apache-2.0 | |
| // SPDX-License-Identifier: MIT |
There was a problem hiding this comment.
Thanks I need go through and complete this
There was a problem hiding this comment.
Yeah, Moonstream contracts were Apache-2.0 but we are using MIT for the protocol contracts.
web3/flows/DropperV3.md
Outdated
|
|
||
| #### `DropperV3Facet-1`: Anybody should be able to deploy a Dropper contract. | ||
|
|
||
| Any account should be able to deploy a `DropperV3Facet` contract. The constructor should not revert. |
There was a problem hiding this comment.
question: Is this gonna be filled?
web3/hardhat.config.ts
Outdated
| overrides: { | ||
| "contracts/security/terminus/TerminusFacet.sol": { | ||
| version: "0.8.24", | ||
| settings:{ | ||
| optimizer: { | ||
| enabled: true, | ||
| runs: 200 | ||
| }, | ||
| }, | ||
| }, | ||
| "contracts/drops/dropper-V3/DropperV3Facet.sol" : { | ||
| version: "0.8.24", | ||
| settings: { | ||
| optimizer: { | ||
| enabled:true, | ||
| runs:200 | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
issue (blocking): I think we can add an optimizer globally instead for just two files
There was a problem hiding this comment.
This isn't recommended for default since optimizing can introduce bugs in seemingly correct code. So our default shouldn't be to optimize contracts. The Terminus Contract and DropperV3Facet need the optimization do to contract size.
There was a problem hiding this comment.
I really prefer not having the optimizer enabled. Do we need the optimizer for these facets?
There was a problem hiding this comment.
The optimizer is needed for the terminus contract. I can look into the terminus to see if there is any changes I could make.
Migration of dropper v0.2.0 from Moonstream
Migration of Terminus from Moonstream
Migration of Diamonds from Moonstream
Creation of Dropper v0.3.0