SIP 89 Virtual Synths #821
Conversation
|
Example of running
at the same time, here was the rate on 1inch
|
# Conflicts: # package-lock.json # publish/src/commands/fork.js
# Conflicts: # contracts/Exchanger.sol # package.json # publish/ovm-ignore.json # publish/releases.json # test/publish/index.js
# Conflicts: # package-lock.json # package.json
| } | ||
| } | ||
|
|
||
| function _createVirtualSynth( |
There was a problem hiding this comment.
Is the concept with ExchangerWithVirtualSynth that overrides the internal function _createVirtualSynth in the Exchanger class supposed to be that the former will be whats deployed on L1 and the normal "Exchanger" contract will be only be on OVM ?
| ); | ||
|
|
||
| _processTradingRewards(fee, originator); | ||
|
|
||
| _emitTrackingEvent(trackingCode, destinationCurrencyKey, amountReceived); | ||
| } | ||
|
|
||
| function exchangeWithVirtual( |
There was a problem hiding this comment.
I'd split the VirtualSynth creation outside of the internal _exchange function for separation of concerns so that exchangeWithVirtual() would create a VirtualSynth after doing the exchange / conversion instead of relying on a bool flag to determine if a VirtualSynth is created within the internal _exchange function.
This could be.a limitation with the stack too deep issue instead of returning the required variables to setup a virtual synth not inside _exchange ?
There was a problem hiding this comment.
I was thinking this too; however the destination account needs to be known before the exchange (which is either the vSynth or the original recipient), and the vSynth needs to know the amount received after the exchange.
It's possible to do this by breaking the _exchange function up into the bits before the call to _convert, and the bits after, but I'm not sure this is any cleaner than what's here. Perhaps we can just do the exchange quantity calculation by itself and construction of the vSynth before rather than after the exchange itself, and pass these in as parameters?
There was a problem hiding this comment.
Yeah Anton's point answers your Jacko. But doing the creation first could definitely work and might be a lot nicer. Let me look into this.
|
This needs to be moved to a PR with a new branch name to trigger the |
|
|
||
| if (virtualSynth) { | ||
| vSynth = _createVirtualSynth(dest, recipient, amountReceived); | ||
| dest.issue(address(vSynth), amountReceived); |
There was a problem hiding this comment.
It's a bit of a nitpick, but a minor amount of space can be saved by reassigning the recipient address and using only one issue call, as follows:
if (virtualSynth) {
recipient = address(_createVirtualSynth(dest, recipient, amountReceived);
}
dest.issue(recipient, amountReceived);There was a problem hiding this comment.
Alternately, _createVirtualSynth on this layer could just return the recipient or zero address, and then the if statement can be removed, and the virtualSynth bool argument removed entirely in the enclosing call to _exchange by relying on the function overriding. E.g.:
vSynth = _createVirtualSynth(dest, recipient, amountReceived); // on layer 2 returns address(0);
address recipientOrSynth = vSynth == address(0) ? recipient : vSynth;
dest.issue(recipient, amountReceived);| ); | ||
|
|
||
| _processTradingRewards(fee, originator); | ||
|
|
||
| _emitTrackingEvent(trackingCode, destinationCurrencyKey, amountReceived); | ||
| } | ||
|
|
||
| function exchangeWithVirtual( |
There was a problem hiding this comment.
I was thinking this too; however the destination account needs to be known before the exchange (which is either the vSynth or the original recipient), and the vSynth needs to know the amount received after the exchange.
It's possible to do this by breaking the _exchange function up into the bits before the call to _convert, and the bits after, but I'm not sure this is any cleaner than what's here. Perhaps we can just do the exchange quantity calculation by itself and construction of the vSynth before rather than after the exchange itself, and pass these in as parameters?
| emit VirtualSynthCreated(address(vSynth), address(synth), synth.currencyKey(), amount); | ||
| } | ||
|
|
||
| event VirtualSynthCreated(address vSynth, address synth, bytes32 currencyKey, uint amount); |
There was a problem hiding this comment.
Does this strictly need the currency key? One assumes it could always be recovered from the synth itself at the block this was emitted.
There was a problem hiding this comment.
No not strictly - it just seemed like it might be better for reporting - but def not critical.
contracts/VirtualSynth.sol
Outdated
| synth = _synth; | ||
| resolver = _resolver; | ||
|
|
||
| // Note: we can do this as Exchanger will issue this amount to us |
There was a problem hiding this comment.
I'm not sure this comment is super clear. I guess it's saying that once the exchange that created this vSynth is complete, _synth.balanceOf(address(this)) == _amount?
contracts/VirtualSynth.sol
Outdated
| return string(abi.encodePacked("v", synth.currencyKey())); | ||
| } | ||
|
|
||
| function decimals() external pure returns (uint8) { |
There was a problem hiding this comment.
Do we need both a public DECIMALS constant and this external decimals function? Isn't renaming DECIMALS to decimals sufficient to satisfy the ERC20 extended interface? Or am I missing some integration that expects the DECIMALS form?
There was a problem hiding this comment.
I was just following the convention in Synth.sol but yeah, different scenario. I'll fix it up.
|
Closed for #857 |
No description provided.