-
Notifications
You must be signed in to change notification settings - Fork 240
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
Problem: not possible to cancel SendToEthereum transactions (fixes #389) #532
Problem: not possible to cancel SendToEthereum transactions (fixes #389) #532
Conversation
Codecov Report
@@ Coverage Diff @@
## main #532 +/- ##
==========================================
+ Coverage 39.91% 41.83% +1.92%
==========================================
Files 30 30
Lines 1596 1690 +94
==========================================
+ Hits 637 707 +70
- Misses 913 929 +16
- Partials 46 54 +8
|
2ef2d4d
to
bcea008
Compare
bcea008
to
e46f813
Compare
contracts/src/ModuleCRC21.sol
Outdated
} | ||
|
||
// send to ethereum through gravity bridge | ||
function send_to_ethereum_v2(address recipient, uint amount, uint bridge_fee) external { |
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.
Without having to migrate all existing contract, it is possible to create a wrapper over the CRC20 to be able to call __CronosSendToEthereum
and have a separate supply from the original CRC20
It would be cleaner to remove send_to_ethereum
from the CRC20.sol and remove the _v2 here however this might change the bytecode of the original CRC20 standard (even though nobody is using this method 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.
Agree with the standard extension, and deprecate the send_to_ethereum
from the original one.
But existing contracts still need to migrate code to support the gravity bridge, right?
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.
another idea is to have a wrapper over exsting CRC20 contract to be able to add the ability to bridge.
For example having ETH_VVS
will be a wrapper of VVS.sol
and expose send_to_ethereum
and wrapping/unwrapping
functions
The 1-1 supply control will be guarantee by the smart contract
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'd suggest documenting these architectural decisions: https://github.com/crypto-org-chain/cronos/blob/main/docs/architecture/adr-template.md + getting some feedback from dApp developers on it
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.
another idea is to have a wrapper over exsting CRC20 contract to be able to add the ability to bridge. For example having
ETH_VVS
will be a wrapper ofVVS.sol
and exposesend_to_ethereum
andwrapping/unwrapping
functions The 1-1 supply control will be guarantee by the smart contract
it's still a contract migration, the client need to use a different contract address, but internally it reuse the old contract to avoid state migration, right?
I think dapps could have multiple solutions to do this, sometimes even have the upgradability build-in.
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 client need to use a different contract address, but internally it reuse the old contract to avoid state migration
Why does it need to use old contract? Mapping between gravitydenom <-> contract address will be done on the wrapped contract.
The drawback is that to be able to send to ethereum, one needs to wrap its token first (or unwrap for the opposite direction) but that can be integrated seeminglessly by the Dapp
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 see, you mean a 1-to-1 token wrapping contract, I guess for erc20, user will need to do a) approve, b) swap, c) send_to_ethereum, right?
680cae3
to
525befb
Compare
2de2f3f
to
8559d28
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.
LGTM in general, only some minor suggestions.
3ccc45e
to
907e625
Compare
this is modified after running |
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.
LGTM
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
Solve this issue #389
PR Checklist:
make
)make test
)go fmt
)golangci-lint run
)go list -json -m all | nancy sleuth
)Thank you for your code, it's appreciated! :)