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

add derives for transfer #6799

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gregdhill
Copy link
Contributor

Signed-off-by: Gregory Hill [email protected]

@lgtm-com
Copy link

lgtm-com bot commented Jan 7, 2022

This pull request introduces 1 alert when merging 203ce60 into 2eea385 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Signed-off-by: Gregory Hill <[email protected]>
@lgtm-com
Copy link

lgtm-com bot commented Jan 7, 2022

This pull request introduces 1 alert when merging 75fa6ce into 2eea385 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

Comment on lines +71 to +73
if (!api) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though there is a check for the api not to be undefined before calling this function, unless this if is added the app crashes

Copy link
Member

@jacogr jacogr Feb 7, 2022

Choose a reason for hiding this comment

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

The signature for all derives are -

// ReturnType is here for illustrative purposes
type ReturnType = string;

function myDerive (instanceId: string, api: ApiInterfaceRx): Observable<ReturnType> {
  return memo(instanceId, (): Observable<ReturnType> => {
    // we work with observables here
    return api.rpc.chain.getHeader().pipe(
      switchMap((header) => of(header.toString()))
    );    
  });
}

So in your case you received 2 params, not 3 (and not an ApiPromise instance, they don't exist in derives)

Comment on lines +233 to +237
: !!api && isFunction((api.derive.balances as any).transferAll)
? transferBalance
: isProtected
? transferBalance
: transferBalance
Copy link
Contributor

Choose a reason for hiding this comment

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

Although the derive is defined in the apps-config for interBtc, it's empty when used here. transferBalance is what the derive is supposed to be, and it works this way. How can this be correctly implemented using the derive?

@daniel-savu
Copy link
Contributor

@jacogr sorry for the tag, not sure if you get notifications from the comments above and I can't mark the PR from 'draft' to 'ready' myself.

Do you have any tips for fixing those two problems?

@gregdhill gregdhill marked this pull request as ready for review January 20, 2022 12:41
@nud3l
Copy link
Contributor

nud3l commented Feb 7, 2022

@jacogr would be great to get your feedback on the two questions above. We'd like to resolve those to make it possible to transfer tokens that are implemented via orml-tokens.

@jacogr
Copy link
Member

jacogr commented Feb 11, 2022

I did comment above, the TL;DR is that you need to use a derive interface with an ApiInterfaceRx signature to make it work.

@daniel-savu
Copy link
Contributor

Thank you! I'll take a look as soon as I get a chance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants