-
Notifications
You must be signed in to change notification settings - Fork 36
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
upgradable wCCD token #128
Conversation
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 overall it's looking good, but I am a bit confused by the state of it.
Are the TODOs meant to be resolved now?
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.
Looks like a good start.
As a general comment:
Please be explicit about what you want a review of when requesting reviews for WIP PRs 😊 You could fx write it in the PR description.
I plan to address all of the TODOs as part of this pull request eventually. I am working on the remaining TODOs. |
62bfdd0
to
9e550cf
Compare
e80fc43
to
e4e4902
Compare
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 looks overall quite good, although I have not managed to get through all the details of state management between the 3 contracts. I have the following high-level comments
- It would be useful to have a more detailed overview of the 3 contract interactions, who holds what, and what the flow of data and tokens is.
- There was some discussion about the need to have the "unpause" time as opposed to just a flag of "locked/unlocked". It seemed to me like agreement was not fully reached. Could you provide rationale in comments in the code for the choice of the timestamp solution?
/// Some additional bytes to include in the transfer. | ||
data: AdditionalData, |
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.
Do we make use of this? For the hook? Perhaps the comment could be updated to clarify.
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.
Updated the two comments about additionaData
to the following:
- If the
Receiver
is a contract the unwrapped CCD together with these additional data bytes are sent to the function entrypoint specified in theReceiver
. - Some additional bytes are used in the
OnReceivingCis2
hook. Only if theReceiver
is a contract and theReceiver
is not the invoker of the wrap function the receive hook function is executed.
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 looks quite good to me. I agree with Ales about the overview and unpause time. And I still think we should consider using this pattern for all the states: enum State_X { Uninitialized, Initialized { proxy_address: ContractAddress, ... }}
.
189351a
to
ff6f810
Compare
7ad7edb
to
d36702f
Compare
let receive_address = input.params.to.address(); | ||
|
||
// Proxy holds CCD funds. CCD is sent to proxy. | ||
host.invoke_contract_raw( |
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.
Instead of moving the CCD back and forth between proxy and implementation, it might be enough to just pass the information back removing the need to call back the proxy with the CCD.
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.
The CCD amount is passed by the fallback function on the proxy to the implementation. The implementation has the logic to check how much WCCD should be minted (based on the amount of CCD it receives in the wrap
function call). This logic has to be executed before the CCD can be sent to the proxy (sent to be stored on the proxy). The CCD should be stored on the proxy in case we upgrade the implementation.
Can you please explain how you can pass the information back or improve the pattern based on your above comment?
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 was thinking of adding the amount of tokens (just like the sender address), such that the information of how much was transferred is forwarded to the implementation, but not the actual CCDs.
But I am not sure whether it will simplify anything.
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.
Since the fallback function will be used by all state-mutative functions, we would also append the "amount of tokens" to the "transfer", "unwrap", and "updateOperator" functions which do not need that information. I am not sure what is the best (most efficient/ simplest) way of handling it.
a974742
to
9caba73
Compare
I guess we should close this and thank Doris for all of her hard work proving why we really needed the native upgradability 😊 |
Closing this, but keeping the branch alive in case we need to come back to some details here. |
Purpose
Implementing an upgradable wCCD token
closes #129
Changes
Adding admin addresses that are authorized to do maintenance tasks on the smart contracts.
Adding the capability to pause the contract by the admin address.
Adding a pattern to deploy and link up the
proxy
,implementation
andstate
contracts.Adding a pattern to upgrade the implementation contract.
Adding a pattern that all wccd functions (e.g.
wrap
,unwrap
,transfer
,updateOperator
,balanceOf
,operatorOf
,supports
andtokenMetadata
) have to be invoked on theproxy
contract.Adding a pattern to store ccd funds on proxy.
Adding events/logs to proxy
URL can be set by admin.
Checklist
hard-to-understand areas.