Skip to content
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

Deposit instruction based on deposit rules #877

Conversation

GhenadieVP
Copy link
Contributor

@GhenadieVP GhenadieVP commented Oct 30, 2023

Jira issue: https://radixdlt.atlassian.net/browse/ABW-2361, https://radixdlt.atlassian.net/browse/ABW-2516

When transferrin between user accounts, determine the deposit instruction(and by implication the requirement of additional signature) based on the account deposit rule.
If an user owned account, they are about to deposit into, would allow the deposit to happen without requiring a signature the Wallet will not add it.

Demos

AcceptAll deposit rule:

  • Resource has no exception -> try_deposit_or_abort instruction is used.
  • Resource has deny exception -> deposit instruction is used.
RPReplay_Final1698693118.mov

AcceptKnown deposit rule

  • Resource is known to the account and it has no exception -> try_deposit_or_abort instruction is used.
  • Resource is known to the account and it has deny exception -> deposit instruction is used.
  • Resource is not known to the account and it has no exception -> deposit instruction is used.
  • Resource is not known to the account and it has allow exception -> try_deposit_or_abort instruction is used.
RPReplay_Final1698693407.mov

DenyAll deposit rule

  • Resource has no exception -> deposit instruction is used.
  • Resource has allow exception -> try_deposit_or_abort instruction is used.
RPReplay_Final1698693239.mov

@GhenadieVP GhenadieVP changed the title Signatures based on deposit rules Deposit instruction based on deposit rules Oct 31, 2023
@GhenadieVP GhenadieVP marked this pull request as ready for review October 31, 2023 06:39
Copy link
Contributor

@kugel3 kugel3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github seems to think there is an error in a test

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! Need to change tests to use withTimeLimit, also what is the plan for:

	// TODO: Temporary revert of checking if the receiving account is a ledger account
		let isSoftwareAccount = true // !receivingAccount.isLedgerAccount

not to be addressed by this PR? do we have another ticket for that? or should we not at all have logic be dependent on if ledger or not?

Copy link
Contributor

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, good job!