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

Wallet native logic #27

Merged
merged 20 commits into from
Aug 26, 2020
Merged

Wallet native logic #27

merged 20 commits into from
Aug 26, 2020

Conversation

abefernan
Copy link
Collaborator

@abefernan abefernan commented Aug 25, 2020

Closes #7

@abefernan abefernan self-assigned this Aug 25, 2020
@abefernan abefernan marked this pull request as ready for review August 25, 2020 13:11
@ethanfrey
Copy link
Member

Improve error handling on sendTokens result, but sendTokens seems to throw no error.

Check if the result has a code. Code 0 or undefined is success. Other code is an error (use log)

  public async postTx(tx: StdTx): Promise<PostTxResult> {
    const result = await this.lcdClient.postTx(tx);
    if (!result.txhash.match(/^([0-9A-F][0-9A-F])+$/)) {
      throw new Error("Received ill-formatted txhash. Must be non-empty upper-case hex");
    }

    return result.code !== undefined
      ? {
          height: Uint53.fromString(result.height).toNumber(),
          transactionHash: result.txhash,
          code: result.code,
          rawLog: result.raw_log || "",
        }
      : {
          logs: result.logs ? parseLogs(result.logs) : [],
          rawLog: result.raw_log || "",
          transactionHash: result.txhash,
          data: result.data ? fromHex(result.data) : undefined,
        };
  }

@ethanfrey
Copy link
Member

ethanfrey commented Aug 25, 2020

Example how to check for errors:

try {
  const res = await client.sendTokens(rcpt, [{denom: "ushell", amount: "123456"}]);
  if (res.code) {
    throw new Error(`sendToken error: ${res.log}`);
  }
  // handle success case
} catch (e) {
  // handle error case
}

You can do more introspection with the result when res.code is truthy to build clearer error messages

@ethanfrey
Copy link
Member

ethanfrey commented Aug 25, 2020

Reviewed the UI by clicking around. And actually had a hard time to get send to error... validation logic and all.

Places to improve:

  • Rather than 79000 ushell, please show 0.079SHELL. For the mapping, just define some mapping in the config. Please look at BankMeta in faucet for an example of this metadata. Here is how they build a Decimal using the balance and the metadata

  • On the Send page, it is very helpful to show the current balance, so I can remember what I have available to send.

I do actually get a "nice" error message now: "Send transaction failed:insufficient funds: insufficient account funds; 72994ushell < 74000ushell: failed to execute message"

I will take a look at the code, but the flow is working pretty well. Mainly changing ushell to SHELL. (And don't forget to do the ureef -> REEF mapping as well)

@ethanfrey
Copy link
Member

Also, tried search. It doesn't change anything. I tried the following accounts (which both exist on coralnet):

  • coral1r4tnas9cyfjvsq5yuusrlmvzxr6h7wu9srjxfd
  • coral17qgdxytgygz54etpq4nkmptfl5c3rl8z3sfxxa

Please check the search field

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good besides my comments

readonly httpUrl: string;
readonly feeToken: string;
readonly gasPrice: number;
readonly httpUrl?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Why all optional? Does this make sense if it misses httpUrl or such fields?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd need to know which ones can be optional, I just needed codeId to be optional at the moment

@@ -0,0 +1,26 @@
import { useSdk } from "@cosmicdapp/logic";
Copy link
Member

Choose a reason for hiding this comment

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

Stuff that can move to design in a future PR, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would move ProtectedSwitch to logic, since it does not have to do with layout or styling

getClient()
.sendTokens(recipientAddress, transferAmount)
.then((result) => {
//Rudimentarily check if rawLog is error
Copy link
Member

Choose a reason for hiding this comment

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

Check for if (result.code) { here. We can definitely not rely on the "github.com" error message. Soon we will turn off all the stack trace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the source of my confusion was that result is PostTxResult but it does not give me access to the code field. I see now that I have 2 type guards: isPostTxSuccess and isPostTxFailure, that let me narrow the type. However, I'm guessing that I can just do if (isPostTxFailure(result))

}

// eslint-disable-next-line react/display-name
const Input = React.forwardRef((
Copy link
Member

Choose a reason for hiding this comment

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

I was looking for where this was used (to see the search logic) and couldn't find it anywhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry for wasting your time looking for the logic, I was not 100% sure about the flow and so it was not implemented yet. I had thought about pre-filling the searchbar with your own address and letting you change it to check other addresses' balance, but the flow described in #28 looks much better

@ethanfrey
Copy link
Member

Please just fix the logic changes and do not worry about the UI stuff I mentioned above. I made another issue to rethink those flows: #28

I think the Native Token and the CW20 Token designs are more different than I thought. And you made some good points when we talked.

@abefernan abefernan merged commit fcb7e58 into master Aug 26, 2020
@abefernan abefernan deleted the wallet-native-logic branch August 26, 2020 10:12
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Propose some improvements to the coin map logic

[key: string]: MappedCoin;
}

function mapCoin(coin: Coin, coinMap: CoinMap): Coin {
Copy link
Member

Choose a reason for hiding this comment

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

I see what you do here, and in the config and it seems to work.
Seems a bit odd going back and forth and back again.

I would just store a map native token -> display token, and always pass the native token (ushell) as arguments internally. Then you can just do the forward mapping for the display.

let mappedAmount = "0";

if (mappedCoin.fractionalDigits > 0) {
mappedAmount = Decimal.fromAtomics(coin.amount, mappedCoin.fractionalDigits).toString();
Copy link
Member

Choose a reason for hiding this comment

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

You can remove a few lines above and below and just use this for the forward mapAmount

};

const coinMap: CoinMap = {
ushell: { denom: "SHELL", fractionalDigits: 6 },
Copy link
Member

Choose a reason for hiding this comment

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

I would keep ushell and ureef and remove the other two. Going back and forth risks loosing precision.
(In fact if this ever ends up as a number, this is only safe if we have positive fractionalDigits)

@abefernan abefernan mentioned this pull request Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add wallet logic
2 participants