Skip to content
This repository has been archived by the owner on Jun 16, 2022. It is now read-only.

isValidTransaction dropped in favour of checkCanBeSpent for custom validations #1449

Merged
merged 1 commit into from
Sep 25, 2018

Conversation

juan-cortes
Copy link
Contributor

@juan-cortes juan-cortes commented Aug 23, 2018

Simple fix for #1448 Maybe I misunderstood the function of isValidTransaction but since it wasn't being called I figured it's probably a bug.

@gre
Copy link
Contributor

gre commented Aug 24, 2018

mmh I think we should probably be able to just use checkCanBeSpent instead (and probably provide a PR that drops isValidTransaction that was way too limited. the fact it's sync and also just return a boolean. checkCanBeSpent is its "async" version that also throw custom errors – that can be translated)

@juan-cortes
Copy link
Contributor Author

juan-cortes commented Aug 24, 2018

After our talk @gre I've changed the PR to drop isValidTransaction and renamed checkCanBeSpent to a more generic checkValidTransaction. Had to make some small changes to the bridges in order to allow checkValidTransaction to return false for validations of the memo. Hopefully is not too intrusive.

Basically, the promise returned (implicit or explicit) should return a boolean since if we only rely on throwing errors or not I can't disable the continue button while handling the validation on a different input.

@juan-cortes juan-cortes force-pushed the issue-1448 branch 3 times, most recently from 1c840b6 to 6461510 Compare August 24, 2018 11:14
Renaming checkCanBeSpent to checkValidTransaction, dropping isValidTransaction

Added the isZero check from the removed method, removed the dead code from bridges
juan-cortes added a commit to juan-cortes/ledger-live-desktop that referenced this pull request Aug 25, 2018
@juan-cortes juan-cortes changed the title Fix Issue 1448 isValidTransaction dropped in favour of checkCanBeSpent for custom validations Aug 25, 2018
Copy link
Contributor

@gre gre left a comment

Choose a reason for hiding this comment

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

LGTM, i'll give a quick try on monday

checkCanBeSpent(account: Account, transaction: Transaction): Promise<void>;
// validate the transaction and all currency specific validations here, we can return false
// to disable the button without throwing an error if we are handling the error on a different
// input or throw an error that will highlight the issue on the amount field
Copy link
Contributor

Choose a reason for hiding this comment

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

Please detail why we need the case of returning "false"? I think we always want to give feedback to user with an error (in failing promise) to inform why it's blocking.
and if we don't, we probably just should ignore a given error to not display it.

NB: i'm currently in progress of refactoring all of these functions in our mobile implementation in order to unify things a bit better between the 2 projects for the future, so we might want to hodl a bit on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I throw an exception on the checkValidTransaction method, the field that gets the feedback and message is the amount field. I wanted to show the error on a different field but still disable the next button if the validation failed.

  checkValidTransaction: async (a, t) => {
    if (t.memoError) {
      return false
    }
    if (t.amount.isLessThanOrEqualTo(a.balance.plus(t.fee))) {
      return true
    }
    throw new NotEnoughBalance()
  }

I'm not against changing this at all, I just needed a way to throw an error on my own input from the AdvancedOptions without it being linked to the amount field. So with the code in this pr, the validation of an invalid form, failing because of a memo issue, would give us this

image

If instead of relying on the false case like I'm doing I throw an error if t.memoError I get this sort of validation which is not really true because I have no issue with the value of amount but rather the value of the memo. Might be a better way that I'm failing to see?

image

@meriadec
Copy link
Member

@gre I think we can merge this for now? If needed we can iterate later for error messages precision.

@gre
Copy link
Contributor

gre commented Sep 19, 2018

we can yes. we need to reiterate around this part for next release so i'll do more testing on these subject anyway. (in context of improving how fees are loaded)

@gre gre merged commit 7236d8c into LedgerHQ:develop Sep 25, 2018
@juan-cortes juan-cortes deleted the issue-1448 branch September 25, 2018 10:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants