-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/mint and call #138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🚀
The only catch is that we rely on ERC677 and not ERC1363.
I know this is a POC, so e.g. the withdraw
method is very simple. I have some ideas on how you can allow each investor to claim their "portion" of the funds in the wallet. To be discussed during our next call 😀
contracts/Wallet.sol
Outdated
IERC20(fundraising.currency()).approve(address(fundraising), amount); | ||
// todo: add try catch https://solidity-by-example.org/try-catch/ | ||
fundraising.buy(amount, tokenReceiver); | ||
return 0x600D600D; // ERC1363ReceiverSuccess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to @CJentzsch comment above ERC677 expects a bool to be returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERC677 seems abandoned to me. The last comments on the (closed) issue indicate that is will not be continued, partially because EIP1363 solves all problems ERC677 addressed. EIP1363 is final. Sadly, it is the more complex solution, but it is a good solution non the less. I don't like to keep poking the dead ERC677 horse with a stick to make it move. I missed parts of the thought process that lead to the decision for ERC677 though. @CJentzsch, @gislik are there good reasons for ERC677 and against EIP1363? |
Bumps [decode-uri-component](https://github.com/SamVerschueren/decode-uri-component) from 0.2.0 to 0.2.2. - [Release notes](https://github.com/SamVerschueren/decode-uri-component/releases) - [Commits](SamVerschueren/decode-uri-component@v0.2.0...v0.2.2) --- updated-dependencies: - dependency-name: decode-uri-component dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [cookiejar](https://github.com/bmeck/node-cookiejar) from 2.1.3 to 2.1.4. - [Commits](https://github.com/bmeck/node-cookiejar/commits) --- updated-dependencies: - dependency-name: cookiejar dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
we will not implement this now |
Testing first specification of
mintAndCall
for usefulness.Run the first test like this:
forge test --match-path test/BuyWithMintAndCall.t.sol