[NayNay] File Restructure: Signing Restructure #218
Conversation
- created new file structure for signing flow - updated tui/cli with new changes
There was a problem hiding this comment.
I love the direction and how you've split out commands + types + utils really clearly.
I've listed quite a few smallish things. Sorry it was 2 weeks and now a bunch of change requests, appreciate that might land a bit annoying.
Happy to have a call if you wanna connect + discuss anything (nicer way to resolve opinions sometimes, and can be faster)
src/signing/constants.ts
Outdated
| @@ -0,0 +1,37 @@ | |||
| export const FLOW_CONTEXT = 'ENTROPY_SIGNING' | |||
There was a problem hiding this comment.
🔥 in our design work #94 / https://hackmd.io/g4Tpy6R_Tym-hLUWhlF-ng?view we agreed to call our folders like entropy-sign (and the man-page style name for the command is then entropy-sign)
Request/ Proposal:
- rename folder
entropy-signsign - rename the FLOW_CONTEXT to
ENTROPY_SIGN - rename the class from
SignCommandtoEntropySign
There was a problem hiding this comment.
Still relevant.
Want the folder to reflect the command name (sign, not signing)
There was a problem hiding this comment.
Review 2.
Looking good. There are a few things that are style things (like I want the core functionality all in the main.ts file, not hidden elsewhere, and variable renames), and there are also so gotchas which will cause problems 🔥
The scope creep with new features is starting to burn a bit much.
- I don't want to say again that
msg/msgPathdoesn't work needs more design rawSignis also new (I think)
please revert these. We need this mega branch merged and I don't want to stop to discuss those things 🙏 ❤️
I am real happy to revisit those as simple little side quests after this is merged
NOTE: this PR currently doesn't work with fresh installs! (no accounts = no default selected nor selectable account = throws)
src/common/utils-cli.ts
Outdated
| // .default(process.env.ENTROPY_SESSION) | ||
| } | ||
|
|
||
| export async function loadEntropy (entropy: Entropy | undefined, address: string, endpoint: string, password?: string): Promise<Entropy> { |
There was a problem hiding this comment.
🔥 why does this take entropy as an argument - we don't use it internally for anything... other than reassigning it which ... I don't even know if that does anything outside the scope of this function, ... and if it does ... then maybe don't (mutating inputs is a pattern that will get you burnt)
Strong request to remove entropy as an argument
src/transfer/command.ts
Outdated
| .addOption(passwordOption('Password for the source account (if required)')) | ||
| .addOption(endpointOption()) | ||
| .addOption(currentAccountAddressOption()) | ||
| .action(async (_source, destination, amount, opts) => { |
There was a problem hiding this comment.
🔥🔥🔥🔥🔥 why do you drop the source here ... if I put in that I wanted to move funds from A => B and instead it moved from selectedAccount => B I would be wild mad.
so this might need more design... as the options I can see are:
- A. remove
source - B. require
sourcebut then ... have that override theselectedAccount(error if--accountis provided too?)
There was a problem hiding this comment.
the source in this case must be a funded entropy account, since we require the signer pair, not just an address. The source in this case must be provided by the selected account from the config which can be overrided by the current account address option, so i dont see the need for a specific source argument here
|
TODO:
|
* tweeeeks * more tweeks * fix tests * rename test * fix maskPayload * test fix * change sign command to just return sig
src/transfer/main.ts
Outdated
| }) | ||
| } | ||
|
|
||
| public async sendTransfer (toAddress: string, amount: string, startProgress?: ()=>void, stopProgress?: ()=>void) { |
There was a problem hiding this comment.
This feels like interaction to me... I don't understand why it exists - if the progress things are optional then this wrapper only does
- formatting
amount - logging
I think logging should be in the base transfer anyway ...
| if (transferStatus.isFinalized) { | ||
| if (progress) return progress.stop() | ||
| return | ||
| } |
There was a problem hiding this comment.
Dropped this check as rawTransfer only ever resolves if isFinalized === true already
| const inputAmount = "1.5" | ||
| await run( | ||
| 'transfer', | ||
| TransferService.transfer({ | ||
| from: charlieEntropy.keyring.accounts.registration.pair, | ||
| to: recipientAddress, | ||
| amount: BigInt(1000 * 10e10) | ||
| }) | ||
| TransferService.transfer(naynayAddress, inputAmount) |
There was a problem hiding this comment.
Testing the actual "main" function we're using with the input it will be receiving (e.g. "1.5" as a string)
| @@ -15,19 +15,17 @@ export class EntropyTransfer extends EntropyBase { | |||
| // - progress callbacks (optional) | |||
|
|
|||
| async transfer (toAddress: string, amount: string, progress?: { start: ()=>void, stop: ()=>void }) { | |||
| const formattedAmount = BigInt(parseInt(amount) * 1e10) | |||
| const formattedAmount = BigInt(Number(amount) * 1e10) | |||
There was a problem hiding this comment.
YO! the parseInt here was rounding our inputs.
Also what is our name for this 1e10 ... is that that number of decimal places we support?
Related Issue(s)
Proposed Changes
Testing
Screenshots (if applicable)
Additional Context
Checklist
CHANGELOG.mdentry.github.com:entropyxyz/entropy-docs, where necessary.