-
Notifications
You must be signed in to change notification settings - Fork 241
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: evm contract can't call native module message #45
Merged
Merged
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
9244e34
Problem: evm contract can't call native module message
yihuang 50b765e
run gen-cronos-contracts
yihuang b90b157
rename fee_amount -> bridge_fee
yihuang eff32dd
add evmhooks unit tests
yihuang 85b0d5e
run gen-cronos-contracts
yihuang 9416bab
fix integration test
yihuang 0a4714d
move EvmLogHandler to types module
yihuang bea3c97
move log handlers into separated file
yihuang 30d15bb
ensure the interface
yihuang b492b32
fix comments, const, error messages
yihuang 593189c
add ethereum(gravity bridge) transfer unit tests
yihuang 6c5bd94
rename event
yihuang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,38 @@ | ||
from .utils import ADDRS | ||
from pathlib import Path | ||
|
||
from .utils import ADDRS, deploy_contract | ||
|
||
|
||
def test_basic(cluster): | ||
w3 = cluster.w3 | ||
assert w3.eth.chain_id == 777 | ||
assert w3.eth.get_balance(ADDRS["community"]) == 10000000000000000000000 | ||
|
||
|
||
def test_native_call(cronos): | ||
""" | ||
test contract native call on cronos network | ||
- deploy test contract | ||
- run native call, expect failure, becuase no native fund in contract | ||
- send native tokens to contract account | ||
- run again, expect success and check balance | ||
""" | ||
w3 = cronos.w3 | ||
contract = deploy_contract( | ||
w3, | ||
Path(__file__).parent | ||
/ "contracts/artifacts/contracts/TestERC20A.sol/TestERC20A.json", | ||
) | ||
|
||
amount = 100 | ||
|
||
# the coinbase in web3 api is the same address as the validator account in | ||
# cosmos api | ||
|
||
# expect failure, because contract is not connected with native denom yet | ||
# TODO complete the test after gov managed token mapping is implemented. | ||
txhash = contract.functions.test_native_transfer(amount).transact( | ||
{"from": w3.eth.coinbase} | ||
) | ||
receipt = w3.eth.wait_for_transaction_receipt(txhash) | ||
assert receipt.status == 0, "should fail" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Don't u have to burn the amount?
imo
__CronosNativeTransfer
is not clear for the operation. A transfer imply an origin and a destination.Here it seems that you are either converting or minting?
It seems more that you are con
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 is solely for testing the native call, the semantic of the call is to transfer mapped native token from contract address to recipient.
But because we can't setup token mapping in e2e testing right now, so the test itself is not complete yet (there's a TODO in that).
This is not the e2e testing for the complete bridge yet.
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.
so maybe
__CronosCosmosConvert
or__CronosCosmosMint
?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.
“Native” stands for native token, “Transfer” is also self explanatory I believe?
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.
is also self explanatory I believe?
I dont think so..
"Native" has barely any meaning without context. And using "Transfer" without specifying a sender and destination sounds weird.
If we can't find a proper short term to describe the operation, I would prefer to be very explicit and call the event
"__CronosSendFromContractToAccount(receiver, amount)" instead...
Mind that most smart contract developer won't have deep knowledge about the internal implementation, or cosmos implementation
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.
When I think about it again, can we say "From" is implied because the action is "Send" and we are executing a contract? It is the "Subject" (what token) and "To" that's unclear.
So
p.s. Having "From" is no harm, my motive is if we think event name is too long then we can think about to trim it or not. I am okay to include it and be more explicit. (Is there any restricting on event name length?)
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 good to me.
And let's use
NativeTokens
forX
for now? orCoins
?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.
Maybe 'X' can be omitted now?
__CronosSendToAccount
__CronosSendToEthereum
__CronosSendToIBCChannel
Even if we add
NativeToken
orCoins
, it is not clear which coin is it. We need a proper term to designate the "Coin associated with the 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.
Looks good too, I suppose we'll have detailed documentation about them as part of the CRC20 standard.
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.
#62
opened issue and documented decision so far in it.