Staker nft #56
Staker nft #56
Conversation
There was a problem hiding this comment.
You need to also commit the corresponding changes to bindings/PositionMetadata/PositionMetadats.go.
web3/contracts/utils/DateTime.sol
Outdated
There was a problem hiding this comment.
This was taken from here: https://github.com/pipermerriam/ethereum-datetime/blob/master/contracts/DateTime.sol
But it has been taken without attribution. I am not ok with this. Either import it as a dependency or put a comment in this file explaining where the code was taken from.
There was a problem hiding this comment.
I would prefer to import it - is there a reason we can't do so (e.g. solidity version issues in the original file)?
There was a problem hiding this comment.
Fairpoint. Thanks for adding.
There was a problem hiding this comment.
From our slack conversation: it's best to keep our compiler uniform! The original file has 0.4.16
web3/test/Staker.test.3.ts
Outdated
| }, | ||
| ], | ||
| }); | ||
| //#endregion | ||
| }); | ||
|
|
||
| it('STAKER-114: The ERC721 representing a non-ERC721 staking position have as its metadata URI a data URI representing an appropriate JSON object', async function () { |
There was a problem hiding this comment.
What about this test? Maybe split STAKER-113 into 4 tests?
There was a problem hiding this comment.
Done:
- Removed
1155,Nativeand20tests - Split the first 2 into their own tests since
20is testSTAKER-114
we are removing generatedefs because it makes no difference and thus takes extra contract bytes.
There was a problem hiding this comment.
@elclandestin0 : Can you remove these changes from your branch? You should be able to do this using the following commands (not tested):
git checkout main -- bindings/WrappedNativeToken/WrappedNativeToken.go
git add bindings/WrappedNativeToken/WrappedNativeToken.go
git commit # type an appropriate commit message into your editor, and hit saveI assume you didn't touch the WrappedNativeToken contract so regenerating the bindings has no meaning.
Do this for all bindings you didn't touch, please.
There was a problem hiding this comment.
These are the only bindings that should remain committed.
|
|
||
| expect(metadata).to.deep.equal({ | ||
| token_id: positionTokenID.toString(), | ||
| image: metadata.image, |
There was a problem hiding this comment.
It would be good to read the expected value from the PositionMetadata contract, and check that the value for the image in the PositionMetdata contract is what is returned from the Staker contract when you call tokenURI.
|
|
||
| it('STAKER-115: `CurrentAmountInPool` and `CurrentPositionsInPool` should accurate reflect the amount of tokens and number of positions currently open under a native token staking pool', async function () { | ||
| it('STAKER-117: `CurrentAmountInPool` and `CurrentPositionsInPool` should accurate reflect the amount of tokens and number of positions currently open under a native token staking pool', async function () { |
There was a problem hiding this comment.
@elclandestin0 : These numbering changes don't match the ones in the flows document.
I am ok with your changes -- maybe just delete the flows document. And let us also remove the graffiti tool. I think that whole system is more trouble than it's worth.
elclandestin0
force-pushed
the
staker-nft
branch
from
95be28e
to
868c1a2
Compare
Staker NFT new metadata
Files added:
DateTime.solFiles edited:
PositionMetadata.solStaker3.test.ts