Display decoded transaction data #102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bonomat
requested changes
Jan 6, 2021
There was a problem hiding this comment.
I don't really like the approach of negative amounts and then re-negating it again in the frontend. I'm afraid that this complexity will bite us in the future. At the very least it should be documented on the function decompose_transaction so that a caller knows what to expect.
What was the particular reason you opted for removing TransactionElements and using negative amounts?
Closed
thomaseizinger
suggested changes
Jan 7, 2021
luckysori
force-pushed
the
display-decoded-transaction-data
branch
from
312cbbb
to
8bc6b8c
Compare
thomaseizinger
approved these changes
Jan 7, 2021
luckysori
force-pushed
the
display-decoded-transaction-data
branch
from
8bc6b8c
to
00d076a
Compare
Also: Fixes #108. Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
luckysori
force-pushed
the
display-decoded-transaction-data
branch
from
00d076a
to
02f89a8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I decided not to integrate with nigiri-chopsticks' or esplora's registry for the purpose of identifying the assets in the transaction because I was struggling to make it work (esplora's returns multiple pages, nigiri-chopsticks' has a different format (without precision), talking to nigiri-chopsticks from the frontend was not working). I'm almost certain it is not worth integrating with nigiri-chopsticks, but we may want to detect when we're in production and figure out how to consume esplora's registry instead of depending on the local registry.
My React is very rusty, so there's probably much nicer ways of doing the same thing.
I am not too happy with the number of things that are being done by the frontend, so I would consider moving almost all the logic into the wallet and changing the API to be something very specific to what we need.
Resolves #95