-
Notifications
You must be signed in to change notification settings - Fork 316
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
test(ICP_ledger): FI-1491: Add tests for existing ledger behavior regarding the anonymous principal #1550
base: master
Are you sure you want to change the base?
Conversation
) | ||
.expect("failed to transfer funds") | ||
.bytes(); | ||
let string_from_bytes_result = String::from_utf8(encoded_transfer_result.clone()); |
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 calling icrc1_transfer
, we should get back a TransferError
in case the transfer failed, but here we need special parsing since we just get back a String
in this error case.
let response = env.execute_ingress_as( | ||
anon, | ||
canister_id, | ||
"transfer", | ||
Encode!(&transfer_args).unwrap(), | ||
); | ||
assert!(response.is_err()); | ||
if let Err(err) = response { | ||
assert_eq!(err.code(), ErrorCode::CanisterCalledTrap); | ||
assert!(err.description().contains("Canister called `ic0.trap` with message: Panicked at 'Sending from 2vxsx-fae is not allowed'")); | ||
} |
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.
Ideally, this response
should be a WasmResult
, in this case containing an icp_ledger::TransferError
, but since the ledger traps, we instead get a UserError
.
assert_eq!(0u64, balance_of(&env, canister_id, anon.0)); | ||
|
||
// Transfer to the account of the anonymous principal succeeds | ||
transfer(&env, canister_id, p1.0, anon.0, TRANSFER_AMOUNT).expect("transfer failed"); |
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.
Should this not also fail?
Otherwise it's essentially an unregistered brun operation.
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 suppose that's TBD - here I'm only adding tests to verify the current behavior.
Co-authored-by: NikolasHai <[email protected]>
Co-authored-by: NikolasHai <[email protected]>
assert!(response.is_err()); | ||
if let Err(err) = response { | ||
assert_eq!(err.code(), ErrorCode::CanisterCalledTrap); | ||
assert!(err.description().contains("Canister called `ic0.trap` with message: Panicked at 'Sending from 2vxsx-fae is not allowed'")); |
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 also worth adding a check that approve by anonymous principal fails.
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.
Done!
…er-behavior-for-anonymous-principal # Conflicts: # rs/ledger_suite/icp/ledger/Cargo.toml
Add tests for transfers to and from the account belonging to the anonymous principal.