method signature lookups, parameter decoding & management #2313
Conversation
# Conflicts: # js/src/abi/spec/function.js
* jg-abi-method-input-decode: decodeInputDat to deocdeCallData functions to allow the decoding of method inputs
* jg-abi-method-input-decode: updated invalid & failing tests don't fail on contract creation encoding return empty when null supplied # Conflicts: # js/src/abi/decoder/decoder.spec.js # js/src/api/util/decode.js
|
It looks like this contributor signed our Contributor License Agreement. 👍 Many thanks, Ethcore CLA Bot |
jacogr
added
the
A3-inprogress ⏳
|
|
||
| import * as abis from '../json'; | ||
|
|
||
| export default class Registry { |
There was a problem hiding this comment.
Why a class here? A simple function returning an object with getContractInstance and lookupAddress would have done the job and is conceptually a lot simpler than a class.
There was a problem hiding this comment.
(same thing for src/contracts/signaturereg.js, src/contracts/tokenreg.js and src/contracts/tokenreg.js)
There was a problem hiding this comment.
On purpose, to lessen congnitive overload. Use classes everywhere, so didn't deviate. I actually realised that with the 3rdparty stuff where we don't, it takes a couple of mins to get into it when changes are to be made.
| import { api } from '../parity'; | ||
| import styles from './identityIcon.css'; | ||
|
|
||
| export default class IdentityIcon extends Component { |
There was a problem hiding this comment.
Why not use src/ui/IdentityIcon?
There was a problem hiding this comment.
Dapps should not rely on the ui/* stuff - we don't build those as a module and would make splitting of the apps from the repo problematic. However, there is probably something to say fro a dapps-ui thing that has all the common components. (e.g. for that IdentityIcon the same code is now in Gavcoin, TokenReg & here - that I know of)
So valid concern, just a different issue.
There was a problem hiding this comment.
* js: add type checking for methodToAbi remove shortcut for non-valid input to decodeCallData
| @@ -16,6 +16,7 @@ | |||
| */ | |||
|
|
|||
| .method { | |||
| font-family: 'Roboto Mono' !important; | |||
There was a problem hiding this comment.
Should be font-family: …, monospace; to tell the computer that you want monospace event if loading the font fails.
There was a problem hiding this comment.
Was discussing it earlier today, really missing Sass variables, these things aren't very DRY. Should probably fix it all-over. Which brings me to the next point, fonts should be bundles as part of Webpack. (Tomasz did it somewhere in one of his apps, everything should be served with Parity, fast, no connection needed to play locally)
|
|
||
| return { | ||
| value: `${ether.toFormat(5)}`, | ||
| token: <small>ΞTH</small> |
There was a problem hiding this comment.
being really picky – As of HTML5, <small> has a semantical meaning and should not be used for stylistic purposes. Use <span.
(also mentioned in https://github.com/ethcore/parity/pull/2099#discussion_r79003278)
There was a problem hiding this comment.
Agreed. As commented there, we need a refactor of these completely - I picked up that habit from the Signer code. In addition, we actually are repeating these things everywhere (not just here), really not a good DRY approach.
|
|
||
| .name, | ||
| .inputs { | ||
| font-family: 'Roboto Mono'; |
There was a problem hiding this comment.
Should be font-family: …, monospace; to tell the computer that you want monospace event if loading the font fails.
* js: user defaults (#2014) Fixing jit feature compilation (#2310) Lenient bytes deserialization (#2036) Fixing tests saturating add Remove crufty code saturating not overflowing Peek transaction queue via RPC (#2270) Avoid penalizing legit transactions Penalize transactions with gas above gas limit Improving txqueue logs
| padding: 1em; | ||
| color: #eee; | ||
| font-size: 0.75em; | ||
| font-family: 'Roboto Mono'; |
There was a problem hiding this comment.
My grumble from https://github.com/ethcore/parity/pull/2313#discussion_r80468994 still applies here afaict.
There was a problem hiding this comment.
Until we have the fonts included, then not an issue anymore. Won't fix.
jacogr
removed
the
A3-inprogress ⏳
There are 2 parts to this -
Future work -
Demos -
Linked issues -
Dependencies -