-
Notifications
You must be signed in to change notification settings - Fork 57
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
New token structure #360
New token structure #360
Conversation
Nice job @kosecki123!
What would you say if we would make proposed improvements modular that sit on top of proposed standard for clarity (examples):
Current draft to be compliant with underlying ERC-1155 needs following:
Regarding compliance with ERC-1888: Awesome work! |
Also you while for your preciseProofs the order is important (as there is a strict scheme) in this case you want to just store balances (order is not important ?), therefore you will need simple merkle proof (without specifying which side of the branch is that), and you can get rid of boolean |
The reason for splitting the private and public functionalities is that, usually the privacy schemes work on a basis of a separate layer where you deposit the tokens to a smart contract from where you can privately move it and then later withdraw it when needed... https://ethresear.ch/t/ethereum-9-send-erc20-privately-using-mimblewimble-and-zk-snarks/6217 |
'0' | ||
) | ||
], | ||
toBlock: undefined |
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.
Please make sure fromBlock
is passed (even if it's 0, so it works on Volta).
Also, maybe instead of toBlock
undefined it's better to say toBlock: 'latest'
?
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 copied from old Certificate.ts, will update.
'0' | ||
) | ||
], | ||
toBlock: undefined |
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.
Consider fromBlock: 0/toBlock latest
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 copied from old Certificate.ts, will update.
from, | ||
to, | ||
parseInt(this.id, 10), | ||
Math.round(amount), |
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.
What is the reason for rounding here?
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.
There's no way to transfer a part of a token in "ERC20". This rounds it to avoid a revert
, but you're right, this might not be desired behaviour.
approved: boolean; | ||
initialized: boolean; | ||
|
||
constructor(id: string, configuration: Configuration.Entity) { |
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.
Constructor could be removed if initialized
declaration would declare its value as well.
|
||
private data: number[]; | ||
|
||
constructor(id: string, configuration: Configuration.Entity) { |
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.
Constructor could be removed if initialized
declaration would declare its value as well.
Aka
public initialized = false
}; | ||
}); | ||
|
||
it('issuer initializes the supply', async () => { |
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.
Empty test
packages/issuer/contracts/Issuer.sol
Outdated
@@ -0,0 +1,229 @@ | |||
// pragma solidity ^0.5.0; |
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 file can be removed.
New Token structure based on ERC-1888
Our current certificate structure is based on ERC-721 non-fungible tokens. This presents an issue when a part of a certificate's volume has to be transferred to another owner.
In cases like these, we currently "split" the certificate into 2 smaller certificates, and then transfer one of the certificates to the new owner, and leave the original certificate to the original owner - deprecating the old certificate.
This approach is not ideal, so we started looking into better ways of changing owners for smaller parts of the certificates.
Decision
We decided to use the ERC-1888 Certificate structure so that we can comply and work on standardizing Certificates.
Consequences
We would no longer need to split certificates and lose the certificate history whenever we want to transfer a part of the certificate to a new owner.
Implemented features
Private issuance- Partially implemented but missing key things