-
Notifications
You must be signed in to change notification settings - Fork 0
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
WIP - New contracts and contracts-sdk packages #1264
base: develop
Are you sure you want to change the base?
Conversation
…pdated contracts-sdk
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.
This was a long review but I don't remember finding anything that requires a change right now. Looks good and I like the write ups and readmes.
|
||
// #0xfE3eEB4ba0FA4BD3b398711AF18908136f5bf430 | ||
export const privateKey1 = EVMPrivateKey( | ||
"87f0ccf57f9778b5b2e7b62ceddd9530dd6daa6efef6cd03c986d4cc48e2d62b", |
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.
These are just keys in Hardhat, right?
protected contractAbi: ethers.InterfaceAbi; | ||
protected hasSigner = false; | ||
|
||
constructor( |
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.
One thing that we decided we did wrong a long time ago was not make a distinction between readable contracts and writable contracts. I think BaseContract here could still be the base, but we remove providerOrSigner and just make it "provider" (no hasSigner). Then we can make BaseWritableContract extends BaseContract and takes a Signer (and a Provider?). Then the hard part though; you can't extend both ReadableFooContract and BaseWritableContract.... Hmm. Never mind I guess for now. Head's kind of foggy
@@ -0,0 +1,427 @@ | |||
import { |
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.
Isn't this just a general ERC1155Contract? I always kind of got confused on the need for a separate "reward" contract version.
@@ -0,0 +1,19 @@ | |||
import { ERC7529ContractError } from "@snickerdoodlelabs/objects"; |
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.
I just realized that you have stuff in src-new; since you moved all the old stuff to a new package contracts-old, shouldn't this just be src?
@@ -0,0 +1,121 @@ | |||
import { |
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.
I think this is part of the old protocol, and won't be needed in the new contracts package
@@ -0,0 +1,16 @@ | |||
import { BigNumberString, EVMAccountAddress } from "@snickerdoodlelabs/objects"; |
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.
I think this can go away as well, stake for rank is currently dead.
public abi?: string, | ||
) {} | ||
|
||
wait(): ResultAsync<ethers.TransactionReceipt, TransactionResponseError> { |
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.
need "public" here
@@ -0,0 +1,1170 @@ | |||
export default { |
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.
You should remove these ABIs, consent contracts are not part of the system anymore.
|
||
### [token](/packages/contracts/contracts/token/README.md) | ||
### [user-wallet](/packages/contracts/contracts/user-wallet/README.md) |
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.
I'm a little confused, we have the smart-wallet contract above; is that the name we're using, or are we sticking with user wallet?
/// @param to The receiver address | ||
/// @param amount The amount to transfer in wei | ||
function transferERC20(address tokenAddress, address to, uint256 amount) public onlyOwner { | ||
require(IERC20(tokenAddress).transfer(to, amount), "ERC20 transfer failed"); |
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.
These methods I thought were going to be "withdraw" methods
…eturn OperatorAndPoint object
…net readme, tasks
…ickerdoodleLabs/protocol into feature/new-contracts-package
@@ -10,7 +10,7 @@ WORKDIR /root | |||
COPY . . | |||
|
|||
# install contracts package dependencies | |||
RUN npm install | |||
RUN npm install -D @nomicfoundation/hardhat-toolbox solidity-coverage dotenv | |||
RUN npm install --force |
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.
needs force otherwise it errors with ethers version mismatch
…ickerdoodleLabs/protocol into feature/new-contracts-package
Release Notes
JIRA Link
Summary:
Includes new contracts and contract-sdk packages for the revamped contracts.
Intended results:
Potential Failures:
Relevant Metrics/Indicators:
Testing Notes:
Pre-Flight Checks