Conversation
nuevoalex
requested review from
KillariDev and
MicahZoltu
and removed request for
KillariDev and
MicahZoltu
MicahZoltu
requested review from
KillariDev and
MicahZoltu
and removed request for
KillariDev and
MicahZoltu
solidity/contracts/Zoltar.sol
Outdated
|
|
||
| function migrateREP(uint192 universeId, uint256 amount, uint8 outcome) public { | ||
| migrateREPInternal(universeId, amount, outcome, msg.sender, msg.sender); | ||
| function migrateREP(uint192 universeId) public { |
There was a problem hiding this comment.
I wonder if we should rename this to UnwrapForkedRep to be a bit more clear that it is not migrating anything, but instead turning your wrapped legacy REP into REP in each outcome?
| if (singleOutcome < 3 && i != singleOutcome + 1) continue; | ||
| uint192 childUniverseId = (universeId << 2) + i; | ||
| Universe memory childUniverse = universes[childUniverseId]; | ||
| ReputationToken(address(childUniverse.reputationToken)).mint(recipient, amount); |
There was a problem hiding this comment.
This has me wondering if REP should be an ERC1155 token instead of an ERC20, where the token ID is the universe ID. This way there is a single "REP" token address throughout all forks rather than having a new one each time.
There was a problem hiding this comment.
that would be interesting. I am not sure if the UX of its is better for it thought
There was a problem hiding this comment.
Token ID would be universe + outcome I think. Would be easy to do since we have that pattern already established for share tokens. ERC1155 is potentially a UX downgrade for ethereum ecosystem stuff (trading etc) though as it never gained large scale adoption. On the other hand it does mean only one approval for Augur specific UX stuff. I'm ambivalent on it but open to this for sure if we have strong opinions on any level.
solidity/contracts/Zoltar.sol
Outdated
| require(!marketResolutionDataIsFinalized(marketResolutionData), "Cannot migrate REP from finalized market"); | ||
|
|
||
| migrateREPInternal(_universeId, REP_BOND, _outcome, address(this), marketResolutionData.initialReporter); | ||
| migrateREPInternal(_universeId, REP_BOND, address(this), marketResolutionData.initialReporter, 3); |
There was a problem hiding this comment.
Consider using a more obvious sentinel value for this (and make the associated change in the loop below). This will help make it more clear to the reader that this is a sentinel value.
| migrateREPInternal(_universeId, REP_BOND, address(this), marketResolutionData.initialReporter, 3); | |
| migrateREPInternal(_universeId, REP_BOND, address(this), marketResolutionData.initialReporter, type(uint8).max); |
solidity/contracts/Zoltar.sol
Outdated
| Universe memory childUniverse = universes[childUniverseId]; | ||
| ReputationToken(address(childUniverse.reputationToken)).mint(recipient, amount); | ||
| for (uint8 i = 1; i < Constants.NUM_OUTCOMES + 1; i++) { | ||
| if (singleOutcome < 3 && i != singleOutcome + 1) continue; |
There was a problem hiding this comment.
See associated comment above.
| if (singleOutcome < 3 && i != singleOutcome + 1) continue; | |
| if (singleOutcome != type(uint8).max && i != singleOutcome + 1) continue; |
Note: This fails in favor of minting nothing if there is somehow a bug where someone passes a singleOutcome other than the sentinal value or a valid outcome.
solidity/contracts/Zoltar.sol
Outdated
| } | ||
|
|
||
| function migrateStakedRep(uint192 _universeId, uint56 _marketId, uint8 _outcome) external { | ||
| function migrateStakedRep(uint192 _universeId, uint56 _marketId) external { |
There was a problem hiding this comment.
we should use some other term than migrate I feel as we have concept of migrating REP and OI in the higher layer
|
|
||
| function migrateREPInternal(uint192 universeId, uint256 amount, uint8 outcome, address migrator, address recipient) private { | ||
| require(outcome < 3, "Invalid outcome"); | ||
| // singleOutcome will only credit the provided outcome if it is a valid outcome, else all child universe REP will be minted |
There was a problem hiding this comment.
what does this mean? can there be a non-valid outcome?
There was a problem hiding this comment.
Every number in uint8 besides 0, 1, and 2 are "invalid". The code uses "all of the others" as a sentinel value to indicate everything should be minted. My suggestion below is to have a single sentinel value for "mint everything", then 0, 1, 2 would mint a single token, and everything else would just burn the REP without minting anything.
| if (singleOutcome < 3 && i != singleOutcome + 1) continue; | ||
| uint192 childUniverseId = (universeId << 2) + i; | ||
| Universe memory childUniverse = universes[childUniverseId]; | ||
| ReputationToken(address(childUniverse.reputationToken)).mint(recipient, amount); |
There was a problem hiding this comment.
that would be interesting. I am not sure if the UX of its is better for it thought
solidity/contracts/Zoltar.sol
Outdated
|
|
||
| function migrateREP(uint192 universeId, uint256 amount, uint8 outcome) public { | ||
| migrateREPInternal(universeId, amount, outcome, msg.sender, msg.sender); | ||
| function migrateREP(uint192 universeId) public { |
There was a problem hiding this comment.
can you also add amount to this. The second layer controls rep of multiple people so it might sense to migrate only partially, and migrate into multiple universes at once
There was a problem hiding this comment.
Discussed in meeting, leaving out for now but if we can come up with a compelling reason someone would want to only partially convert their REP its easy to add back.
3 participants
No description provided.