-
Notifications
You must be signed in to change notification settings - Fork 0
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
Axelar aleo review #60
base: main
Are you sure you want to change the base?
Conversation
ampd/Cargo.toml
Outdated
aleo-gateway = { workspace = true } | ||
snarkvm-cosmwasm = { workspace = true } | ||
# aleo-parser = { path = "../../axelar-aleo/crates/aleo-parser" } | ||
aleo-parser = { git = "ssh://[email protected]/eigerco/axelar-aleo.git", branch = "aleo-helper-libs" } |
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.
@tilacog how to handle this? including a crate from a private repo
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.
ping @tilacog
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.
We can either try to publish just the aleo-parser
to crates.io, if that's doable; or create a new public repo for it.
I prefer the first option because it's easier to maintain. That crate is very self-contained and don't expose our contract logic, so it should be safe to do so.
We might need to check that with @asmie first.
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.
@tilacog the contract log is not encrypted when your contract is deployed on testnet, so there is nothing to protect when it comes to contract logic
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.
@asmie mentioned we can give access for this repo to eigergo/axelar-aleo
using GitHub tokens/secrets.
This seems like the cleanest approach to me, because wouldn’t have to deal with new repositories or crate publishing.
a909fa9
to
e320d75
Compare
e320d75
to
29dc43e
Compare
contracts/multisig/Cargo.toml
Outdated
@@ -61,12 +62,16 @@ sha3 = { workspace = true } | |||
signature-verifier-api = { workspace = true } | |||
thiserror = { workspace = true } | |||
|
|||
rand_chacha = "0.3" |
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.
check this, or document
} | ||
|
||
#[cfg(test)] | ||
mod tests { |
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.
Add a test for Aleo address
342e5d4
to
8dcfe5d
Compare
63d9105
to
9354ede
Compare
|
||
save_next_verifier_set(deps.storage, &new_verifier_set)?; | ||
// let cur_verifier_set = CURRENT_VERIFIER_SET |
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 need old code? it's in git anyway
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.
Good question: I didn't remove it to be obvious that signer rotation is not implemented. Also the commented out code is the reason why I set some tests as ignore
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.
@anstylian Yeah, I suspected it too, but better to have a simple short explanation why it's commented out
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 will remove it
@@ -425,6 +453,12 @@ mod test { | |||
address_format: AddressFormat::Sui, | |||
should_fail: true, | |||
}, | |||
TestCase { | |||
source_gateway_address: | |||
"aleo1q3t7cjwk9ncxcdxfm8r5ax83mzudd923gffncv5egfjyevfevuyscvcvz".to_string(), |
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.
ok to hardcode?
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.
Thanks for spotting it. It's ok to be hardcoded here because it is the unit test setup. It is the same above for other chains.
@@ -34,6 +34,7 @@ fn verifier_should_not_unbond_while_in_active_set() { | |||
} | |||
|
|||
#[test] | |||
#[ignore] |
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.
if ignore please write why.
Scope - next phase is ok too
@@ -148,6 +149,7 @@ fn claim_stake_when_in_all_active_verifier_sets_fails() { | |||
} | |||
|
|||
#[test] | |||
#[ignore] |
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.
if ignore please write why.
Scope - next phase is ok too
@@ -242,6 +244,7 @@ fn claim_stake_after_deregistering_before_rotation_fails() { | |||
} | |||
|
|||
#[test] | |||
#[ignore] |
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.
if ignore please write why.
Scope - next phase is ok too
@@ -8,6 +8,7 @@ use service_registry_api::msg::QueryMsg as ServiceRegistryQueryMsg; | |||
pub mod test_utils; | |||
|
|||
#[test] | |||
#[ignore] |
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.
if ignore please write why.
Scope - next phase is ok too
@@ -105,6 +106,7 @@ fn verifier_set_can_be_initialized_and_then_manually_updated() { | |||
} | |||
|
|||
#[test] | |||
#[ignore] |
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.
if ignore please write why.
Scope - next phase is ok too
@@ -225,6 +227,7 @@ fn verifier_set_cannot_be_updated_again_while_pending_verifier_is_not_yet_confir | |||
} | |||
|
|||
#[test] | |||
#[ignore] |
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.
if ignore please write why.
Scope - next phase is ok too
@@ -320,6 +323,7 @@ fn verifier_set_update_can_be_resigned() { | |||
} | |||
|
|||
#[test] | |||
#[ignore] |
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.
if ignore please write why.
Scope - next phase is ok too
@@ -369,6 +373,7 @@ fn governance_should_confirm_new_verifier_set_without_verification() { | |||
} | |||
|
|||
#[test] | |||
#[ignore] |
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.
if ignore please write why.
Scope - next phase is ok too
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 will, thanks for the reminder
Description
Todos
Convention Checklist
exported
mod. If those types have already been defined somewhere else, then they should get re-exported in theexported
modSteps to Test
Expected Behaviour
Notes