-
Notifications
You must be signed in to change notification settings - Fork 4
Implement StarknetSetup
to automate setting up chains and IBC for testing
#368
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
Conversation
} | ||
|
||
#[test] | ||
fn test_relay_timeout_packet() -> Result<(), Error> { |
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.
Could we keep this test until it’s migrated to the new format?
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.
Added them back now.
use crate::utils::{init_starknet_bootstrap, load_wasm_client}; | ||
|
||
#[test] | ||
fn test_relay_update_clients() -> Result<(), Error> { |
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.
Could we keep this test until we finish the ICS23 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.
Looks good to me!
assert(token.is_zero(), TransferErrors::TOKEN_ALREADY_EXISTS); | ||
|
||
if !token.is_zero() { | ||
return token.address; |
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.
Here I make token creation to be no-op if a token already exists. This helps resolving the race condition where we try to create a token while a relayer is relaying and triggering the token creation at the same time.
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 race condition makes sense. The refactor is alright for me.
let message = StarknetMessage { | ||
call: Call { | ||
to: ics20_contract_address.0, | ||
selector: selector!("create_ibc_token"), | ||
calldata, | ||
}, | ||
counterparty_height: None, | ||
}; |
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 that you now allow returning existing token. Note that, this is a contract invoke -- so we will spend gas on it, even if we don't modify the state. Don't you think it makes sense to query via ibc_token_address
first ? And create the token if it doesn't exist ?
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.
Ignore!
I see that, you're doing exactly this 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.
Yeah, here I make token creation to be no-op if a token already exists. This helps resolving the race condition where we try to create a token while a relayer is relaying and triggering the token creation at the same time.
use hermes_ibc_test_suite::tests::clearing::TestPacketClearing; | ||
use hermes_ibc_test_suite::tests::transfer::TestIbcTransfer; |
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.
nice ! ✨
Closes: #174
Merge after: informalsystems/hermes-sdk#581