fix: add max token varidation to input form #270
Conversation
|
自分なら || 0 |
|
@taro04
後は、(理想的ではないかもしれないので、)暫定的には、(という感じですが、)送信ボタンをクリックしたときに実行されるメソッドの中で、チェックして、問題がある場合はダイアログを表示してあげるという方針もありかもしれませんね。このあたりも含め検討してみてください。 |
|
次に松岡さんのコメントの通り、 １）についてはuguuだけはsimulationを通すために ２）はエラーメッセージが出るので残高が残高が足りないことは伝わりますが、
|
|
各コインの所持枚数でバリデーション（ただしuguuは-所持枚数-1）を設定しました。
ご確認お願い致します。 |
There was a problem hiding this comment.
@taro04
レビュー遅くなってすみません。
修正したほうが良いと思う点をソースコードにコメントしました。
確認と修正をお願いします。
| } | ||
| return [...coins.keys()].map((index) => { | ||
| let eachAmount = parseInt((coins && coins[index].amount) || '0'); | ||
| if (coins[index].denom === 'uguu') { |
There was a problem hiding this comment.
@taro04
uguuをハードコードして判定するのではなく、minimumGasPricesに含まれているdenomなら、マイナス1するような形の判定に修正すべきかなと思いました。
| if (coins === undefined) { | ||
| return []; | ||
| } | ||
| return [...coins.keys()].map((index) => { |
There was a problem hiding this comment.
@taro04
無理に[...coins.keys()].map((index) => {のようにせずとも、coins.map((coin, index) => {})のようにindexを取って使えるので、そのように実装したほうが自然で情報を失わずに済んで可読性の高い実装になって良いように見えました。
| return []; | ||
| } | ||
| return [...coins.keys()].map((index) => { | ||
| let eachAmount = parseInt((coins && coins[index].amount) || '0'); |
There was a problem hiding this comment.
@taro04
letを使わず、一旦一つのmapで型変換を行い、次にもう一つのmapでマイナス1すべきかどうかチェックしてマイナス1するように実装したほうが自然かつ堅牢かなと思いました。
全体的には以下のようなイメージです。
return coins.map((coin, index) => {
// convert type from string to number
// in this point, keep denom
return numberdCoin;
}).map((numberdCoin, index) => {
// check and if need minus 1
return finalCoin;
})
|
コメントいただいた通り修正しました。 ・処理はmapでつなぐ。 以下ローカルで確認しました。
ご確認お願いいたします。 |
There was a problem hiding this comment.
@taro04
すみません。お願いしていた修正方針が良くなかったように感じていて、改めてソースコード内のコメントを参考に修正方針再検討してもらえないでしょうか？
お手数おかけしてすみませんがよろしくお願いします。
| return []; | ||
| } | ||
| return coins | ||
| .map((coins, _index) => { |
There was a problem hiding this comment.
@taro04
index使わないなら省略可能なので消すのが一般的かなと思いました。
indexを使いたいがために不自然な書き方になっていると思ったのでindexをこう取得して使えるよという例を示しましたが、いらないならば消したほうが良いと思います。
| .map((coins, _index) => { | ||
| return { denom: coins.denom, amount: parseInt((coins && coins.amount) || '0') }; | ||
| }) | ||
| .map((numberedCoin, _index) => { |
There was a problem hiding this comment.
@taro04
index使わないなら省略可能なので消すのが一般的かなと思いました。
indexを使いたいがために不自然な書き方になっていると思ったのでindexをこう取得して使えるよという例を示しましたが、いらないならば消したほうが良いと思います。
| //check whether minimum_gas_prices include this denom | ||
| let isGasPrices: boolean = false; | ||
|
|
||
| this.configS.config.minimumGasPrices.map((minimumGasPrice) => { | ||
| if (numberedCoin.denom === minimumGasPrice.denom) { | ||
| isGasPrices = true; | ||
| } | ||
| }); | ||
|
|
||
| if (isGasPrices) { | ||
| return numberedCoin.amount - 1; | ||
| } else { | ||
| return numberedCoin.amount; | ||
| } |
There was a problem hiding this comment.
@taro04
申し訳ありませんが、自分が修正お願いしていた方針があまりよくなかったように思います。
改めて考えてみた結果、configの中のminimumGasPricesに含まれるか否かという判定条件は適切ではないように感じました。
その瞬間のViewComponentの中で、フォーム内で設定されているgasPriceのdenomが何になっているかを取得して、そのdenomは1引き算するという実装が適切なな気がしています。
(例えばubtcでも手数料払い可能な設定になっていて、実際の送信フォームではuguuで手数料払う設定になっている場合、ubtcは全額送れるはずなのに、全額払う入力でエラーになってしまうのはおかしいような気がしたという感じです。ただ、そもそも1設定しないとダメだったかなという部分に改善の余地があるかもしれません。dummyのGasPriceやFeeは0でダメなの？等...)
また、そもそもここまでの実装では、あくまでもsimulateがエラーなく実行できることしか保証できないように見えていて、ApplicationService内で、simulate後に、送信しようとしている額とsimulateで推定された手数料の合計が、実際の残高を上回っているとエラーになってしまうパターンはケアできていない気がしています。
これらをトータルで評価するには、ApplicationServiceに、残高も渡して、ApplicationService内にて、以下2点をチェックして、それぞれ問題がある場合は、スナックバー等でユーザーに通知してあげたほうが実装しやすく、ユーザーにとってもわかりやすいのでは？とも感じました。
- simulate可能な残高が設定されているか ... gasPriceに指定されたdenomの送信料は、保有残高-1以下になているか
- simulateで判明した手数料と送信残高の合計は、保有残高以下になっているか
全体的に、どう修正するのが良いか、じっくり腰を据えて、実装の改良にご協力もらえると助かります。
お手数ですがよろしくお願いします。
| this.configS.config.minimumGasPrices.map((minimumGasPrice) => { | ||
| if (numberedCoin.denom === minimumGasPrice.denom) { | ||
| isGasPrices = true; | ||
| } | ||
| }); |
There was a problem hiding this comment.
@taro04
値を返して別の配列を作るのではなく何らかの処理を行うだけなら、mapではなくforEachを使うのがこの場合自然かなと思いました。
また、そのような実装方法を取ると、letが必要になってしまいやすいと思います。
例えば今回の場合、以下のようなsomeを使う等で、letを使わず実現できるのではといったことを思いました。(他にも書き方あると思います。)
https://developer.mozilla.org/ja/docs/Web/JavaScript/Reference/Global_Objects/Array/some
場面によってどのような書き方が可読性が高いかという点はケースバイケースで一概には言えないと思うのですが、letはできるだけ使わず、可能な限りconstを使うよう、意識してみてください。(その上でこちらの方が可読性、メンテナンス性高いと思ったということであればすみません。)
There was a problem hiding this comment.
承知しました。javaでArray標準のメソッドはいろいろと便利なものがあるのですね。
forは違うよなと思って、無理やりmap使っていましたが、まさに.some()が探していたものでした。
教えていただきありがとうございます。
|
ご確認ありがとうございます。 現状送信の際の挙動は以下の３パターンです。 このPRでは１）をケアするために最大値-1をバリデーションに設定します。 そもそもsimulationに”1”を設定する必要がある確認したところ、 と実装しているときは何はともあれsimulationを通さないといけないと思っていたのですが、 ここまでレビューいただいて恐縮ですが、以下のように修正しなおそうと思いました。 ・入力バリデーションは単純に所持数（uguu-1しない） このあたりの挙動について違和感ありましたらご教示いただけると幸いです。 |
|
以下の方針で実装しました。 20220202_235505.mp4・動作は実現できていると思います。 |
| maximumGasPrice?.amount === amountGasPrice?.amount && | ||
| amount.denom === minimumGasPrice.denom | ||
| ) { | ||
| const amountForSimulation = parseInt(amount.amount || '0') - 1; |
There was a problem hiding this comment.
uguuの残高がない場合、amountForSimulationがマイナス1になって、simulationがエラーになりそうに見えました。(uguuのFaucetを回さずubtcだけ残高として保有している状態等で発生すると思います。)
これを避けるためには、ひと手間必要なものの、simulationがエラーにならないamount, coins, minimumGasPricesの組み合わせであることを事前にチェックして、エラーになりそうなら、simulationのAPIを叩く前にはじく必要がありそうかなと思いました。(try catchでキャッチしてエラーメッセージは出るのですが、エラーメッセージがCode400みたいな抽象度の高いものになってしまい、詳細がユーザーにわからない状況になってしまうんですよね...)
例えば、一例としては、以下のようなイメージの実装を追加する感じかなと思いました。(alertのところはmat-dialogやsnackbar等で良い感じに実装頂けると助かります。)
const feeDenom = minimumGasPrice.denom;
const simulationFeeAmount = 1;
const tempAmountToSend = amount.find(
(amount) => amount.denom === minimumGasPrice.denom,
)?.amount;
const amountToSend = tempAmountToSend ? parseInt(tempAmountToSend) : 0;
const tempBalance = coins.find((coin) => coin.denom === minimumGasPrice.denom)?.amount;
const balance = tempBalance ? parseInt(tempBalance) : 0;
if (amountToSend + simulationFeeAmount > balance) {
alert(
`Insufficient fee margin for simulation!\nAmount to send: ${amountToSend}${feeDenom} + Simulation fee: ${simulationFeeAmount}${feeDenom} > Balance: ${balance}${feeDenom}`,
);
return;
}
既に実装頂いた内容と近い処理を重複して書いているような気持ち悪さがある気もするので、もっといい書き方もあると思います。そのあたりは適宜チャレンジしてみて頂けると助かります。
| const fixedAmount: proto.cosmos.base.v1beta1.ICoin[] = []; | ||
| amount.forEach((amount) => { | ||
| //has gas denom | ||
| if ( | ||
| maximumGasPrice?.amount === amountGasPrice?.amount && | ||
| amount.denom === minimumGasPrice.denom | ||
| ) { | ||
| const amountForSimulation = parseInt(amount.amount || '0') - 1; | ||
| fixedAmount.push({ amount: amountForSimulation.toString(), denom: amount.denom }); | ||
| } else { | ||
| fixedAmount.push(amount); | ||
| } | ||
| }); |
There was a problem hiding this comment.
間違っているわけではなく好みの問題かもしれませんが、空の配列を新たに定義してpushで詰めていくより、元となる配列からmapで生成した結果として一発で定義したほうが自然かなと思いました。
| const fixedAmount: proto.cosmos.base.v1beta1.ICoin[] = []; | |
| amount.forEach((amount) => { | |
| //has gas denom | |
| if ( | |
| maximumGasPrice?.amount === amountGasPrice?.amount && | |
| amount.denom === minimumGasPrice.denom | |
| ) { | |
| const amountForSimulation = parseInt(amount.amount || '0') - 1; | |
| fixedAmount.push({ amount: amountForSimulation.toString(), denom: amount.denom }); | |
| } else { | |
| fixedAmount.push(amount); | |
| } | |
| }); | |
| const fixedAmount: proto.cosmos.base.v1beta1.ICoin[] = amount.map((amount) => { | |
| //has gas denom | |
| if ( | |
| maximumGasPrice?.amount === amountGasPrice?.amount && | |
| amount.denom === minimumGasPrice.denom | |
| ) { | |
| const amountForSimulation = parseInt(amount.amount || '0') - 1; | |
| return { amount: amountForSimulation.toString(), denom: amount.denom }; | |
| } else { | |
| return amount; | |
| } | |
| }); |
| const isOver = | ||
| parseInt(fee.amount || '0') + parseInt(amountGasPrice?.amount || '0') > | ||
| parseInt(maximumGasPrice?.amount || '0'); | ||
|
|
||
| // ask the user to confirm the fee with a dialog | ||
| const txFeeConfirmedResult = await this.dialog | ||
| .open(TxFeeConfirmDialogComponent, { | ||
| data: { | ||
| fee, | ||
| isTxFeeOver: isOver, | ||
| isConfirmed: false, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
自分自身の好みとしては、ダイアログで警告表示するなら、別のコンポーネントベースのダイアログに分けたほうが色々すっきりするかなという印象です。省エネ的に、他のエラーメッセージと同様に、スナックバーで実装するのもありかなとも思いました。
| <ng-container *ngIf="data.isTxFeeOver; then over; else ok"></ng-container> | ||
| <ng-template #over> | ||
| <div>Review the amount of {{ data.fee.denom }}, you are trying to send.</div> | ||
| </ng-template> | ||
| <ng-template #ok> | ||
| <div>Are you sure you want to send the tx with the fee?</div> | ||
| </ng-template> | ||
| <button | ||
| mat-flat-button | ||
| class="w-2/5" | ||
| color="primary" | ||
| [disabled]="data.isTxFeeOver" | ||
| (click)="okToSendTx()" | ||
| > | ||
| OK | ||
| </button> |
There was a problem hiding this comment.
好みのレベルになる気もしますが、手数料表示とトランザクション実行最終確認と、手数料不足の警告表示(とトランザクション実行止める)のは、ダイアログにするならコンポーネント分けたほうがいいかなと思いました。
(一旦、スナックバーで楽に実装しておくというやり方でもよいかもという気もしています。)
| @@ -12,6 +12,7 @@ export class TxFeeConfirmDialogComponent implements OnInit { | |||
| @Inject(MAT_DIALOG_DATA) | |||
| public readonly data: { | |||
| fee: proto.cosmos.base.v1beta1.ICoin; | |||
| isTxFeeOver: boolean; | |||
There was a problem hiding this comment.
好みのレベルになる気もしますが、手数料表示とトランザクション実行最終確認と、手数料不足の警告表示(とトランザクション実行止める)のは、ダイアログにするならコンポーネント分けたほうがいいかなと思いました。
(一旦、スナックバーで楽に実装しておくというやり方でもよいかもという気もしています。)
| }); | ||
| } | ||
|
|
||
| onMinimumGasDenomChanged(denom: string): void { | ||
| this.selectedGasPrice = this.minimumGasPrices?.find( | ||
| (minimumGasPrice) => minimumGasPrice.denom === denom, | ||
| ); | ||
| this.gasPriceSelected.emit(this.selectedGasPrice); |
There was a problem hiding this comment.
これ、詳細にデバッグできているわけではないのですが、同様の実装を、onMinimumGasAmountSliderChangedに入れる必要はないでしょうか？
スライダーでGas Priceの数値を変更したとき、反映されないような気がしました。
(違ってたらすみません。)
|
コメントありがとうございました。 ０の場合 ユーザーへの表示について現在スナックバーで実装しています。 なお個人的にはユーザーへの異常操作のアナウンスなどは、今後、以下の理由でアラートに統一しても良いのではと思っています。 ・スナックバー：文字が小さい、またユーザーのタイミングで消すことができない （そもそもアラートはデバック用だ、みたいな経緯がありましたらご容赦ください。） |
There was a problem hiding this comment.
概ねOKなのですが、通知に関する以下コメントを受けて、自分でも再確認してみた結果、スナックバーでユーザー自身で閉じるような実装が可能そうに見えたので、コード中のコメント追記した方法(第二引数に閉じるためのボタンのラベル名を文字列で指定してやることで、ユーザー自身で閉じるまで開きっぱなしにできそう)に統一するのが良いかなと思いました。(自分自身、これまで深堀していなかったところで、把握しておらず、すみません。)
このプルリクでは、bank sendに関する部分だけ、エラー通知のスナックバーはユーザー自身が閉じる形に修正して、それ以外のエラー通知のスナックバーをその形に統一するのは、別のプルリクで改めてやるのはどうでしょう？
ご検討ください。
この修正で最後になると思います。色々たくさん検討してもらって感謝です。
・スナックバー：文字が小さい、またユーザーのタイミングで消すことができない
・個別のダイアログ：想定される異常操作ごとにダイアログ作るとコンポーネントが多くなる気がする。ある程度まとめるにしてもどれはまとめるが判断に迷う。
| this.snackBar.open( | ||
| `Insufficient fee margin for simulation!\nAmount to send: ${amountToSend}${feeDenom} + Simulation fee: ${simulationFeeAmount}${feeDenom} > Balance: ${balance}${feeDenom}`, | ||
| ); |
There was a problem hiding this comment.
ユーザーが任意のタイミングでスナックバーをクローズできるようにするには以下のように実装すると可能そうな気がしているので、簡易的にアラート的なユーザー通知を行うには、この書き方に統一するのがよいかもしれませんね。(松岡もこれまであまり詳細調べておらず、このような書き方できるのを、今回コメント頂いて調べて初めて把握しました。意見ご提案感謝です。)
| this.snackBar.open( | |
| `Insufficient fee margin for simulation!\nAmount to send: ${amountToSend}${feeDenom} + Simulation fee: ${simulationFeeAmount}${feeDenom} > Balance: ${balance}${feeDenom}`, | |
| ); | |
| this.snackBar.open( | |
| `Insufficient fee margin for simulation!\nAmount to send: ${amountToSend}${feeDenom} + Simulation fee: ${simulationFeeAmount}${feeDenom} > Balance: ${balance}${feeDenom}`, | |
| 'Close', | |
| ); |
エラー通知系は、一旦これに統一する形で、このプルリクでは、bank send系だけまず修正し、別のプルリクで、全体的に統一する修正を加えてあげるのが良いかなと思いました。
成功通知系は、数秒だったら消えるスナックバーの現状の実装でいいかなと思いました。
|
ご確認、調査いただきありがとうございます。 ご確認お願いいたします。 エラー通知系の修正についてイシュー立てておきます。 |
#262の修正です。
ngFormのinputに[max]を追加する方向で実装進めています。
[max]がstring｜numberしか受け付けないので意外と難しいと感じています。
試したこと
・
"coins[i].amount as string"⇒ html内でas stringが使えない。・/page側でObservable<string[]>作る。 ⇒
|asyncでview側に渡す時点で|nullがつく。ごり押し感がありますが、/view側コンポーネントに
maxAmount: string[]を定義して、for文でcoins?からamountを取り出すしかないかと思っています。
実装の方針について、良い方法ありましたらコメントいただけると幸いです。