SD-102: Refactor fee handling to separate shielding and sending fees#91
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
2d7ce88 to
9ca204f
Compare
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
9ca204f to
9c4ab84
Compare
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
9c4ab84 to
275861b
Compare
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
275861b to
0ca5a62
Compare
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
0ca5a62 to
0e91371
Compare
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
0e91371 to
b8b8652
Compare
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
9c89685 to
fd390c9
Compare
rAskVAL
requested review from
jalooc and
youPickItUp
and removed request for
youPickItUp
| sendingFees: 'sendingFees', | ||
| shieldingFees: 'shieldingFees', | ||
| allowanceFeeEstimate: 'allowanceFeeEstimate', |
There was a problem hiding this comment.
A bit lacking consistency, no? - sometimes calling it just fee, sometimes fee estimate.
Or are the sending and shielding fees really the actual values, not estimates?
| if (!chainId) throw new Error('chainId is not available'); | ||
| if (!walletAddress) throw new Error('walletAddress is not available'); | ||
|
|
||
| const localId = v4(); | ||
|
|
||
| await upsertTransaction(chainId, { | ||
| token: token.isNative ? | ||
| { type: 'native' } : | ||
| { type: 'erc20', address: token.address }, | ||
| token: token.isNative ? { type: 'native' } : { type: 'erc20', address: token.address }, |
There was a problem hiding this comment.
You still have IDE formatting turned on?
There was a problem hiding this comment.
no i've changed it, i think its nicer this way
There was a problem hiding this comment.
Very subjective - e.g. I liked the previous one better ;)
Anyway - when making changes remember that it obfuscates git history, making it harder to trace what and when happened; not to mention that it's an impossible-to-analyse noise for any kind of automatic analysis tools. Sometimes it's justified, sometimes not so much - of course impossible to set a unequivocal general rule for when to do it and when not. But I'd say that making such an non-undoubtedly-useful stylistic change in a code that's completely outside of a scope of the code changes from PR, is definitely too much.
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
863712a to
1418194
Compare
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
1418194 to
58fbc8b
Compare
| shielderPrivateKey: 'shielderPrivateKey', | ||
| tokenSymbol: 'token-symbol', | ||
| wasmCryptoClient: 'wasm-crypto-client', | ||
| } as const; |
There was a problem hiding this comment.
i have sorted them alphabetically
There was a problem hiding this comment.
I don't think that's a good idea - they should be grouped semantically (I think that this won't always be the same as the alphabetical order).
Also this comment applies here.
jalooc
left a comment
jalooc
left a comment
There was a problem hiding this comment.
GTG. Just wrt of this comment, please revert all subjective stylistic changes that were performed on a code that this PR doesn't relate to.
rAskVAL
force-pushed
the
sd-102-refactor-shielding-fees
branch
from
7749ab0 to
6d2d4f1
Compare
2 participants
No description provided.