Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Show contract parameters in MethodDecoding #4024

Merged
merged 6 commits into from
Jan 5, 2017
Merged

Conversation

ngotchac
Copy link
Contributor

@ngotchac ngotchac commented Jan 3, 2017

Closes #3715

Show the contract parameters in MethodDecoding when possible, ie. when the contract has been added from/in the UI. Otherwise, as there is no Constructor signature in the transaction input, it is (AFAICT) impossible to decode the parameters.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 3, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 03e36cc on ng-decode-deployments into ** on master**.

@@ -31,6 +31,12 @@ export default class Param {
}

static toParams (params) {
return params.map((param) => new Param(param.name, param.type));
return params.map((param) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just add a relevant test that verifies both behaviours? (these should be one that just verifies the original)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -534,7 +536,7 @@ class AddressSelect extends Component {
});
}

handleFocus = () => {
handleFocus = (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the (unused) variable here? Consider at least a description name - no e that can be anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to remove this one

const param = this.getParam();

const realValue = value
? (new BigNumber(value))[readOnly ? 'toFormat' : 'toNumber']()
? this.getNumberValue(value)[readOnly ? 'toFormat' : 'toNumber']()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably break when getNumberValue returns from the first test, i.e. where it is null or undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it checks if value is truthy before. Agreed it could be better though

const param = this.getParam();

const realValue = value
? (new BigNumber(value))[readOnly ? 'toFormat' : 'toNumber']()
? this.getNumberValue(value)[readOnly ? 'toFormat' : 'toNumber']()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same breakage comment here since the function may return something without toFormat/toNumber

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

// (minus the bytecode). It seems that they are repeated
// twice
const params = rawInput.slice(codeOffset + rawCode.length);
const paramsBis = params.slice(params.length / 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not 100% sure what paramBis refers to

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the thing is that from what I saw, the transaction input is : somehting . bytecode . constructor_params . constructor_params
Ie. the constructor params are concatenated twice at the end of the input. From what I could find online (which isn't much), it seems that the parameters should appear once. That's why I thought we could try the two cases, and result with which ever is correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm... the 2 concats sounds like a bug on our side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I initially thought, will dig around a bit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we leave this one open until we have resolution or merge and come back to fix the double check when the above is resolved?

(I'm leaning to the former since this is new functionality and would rather get it right first time around, especially since we have code work-arounds in here for the potential issue.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to merge this one first yes

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 5, 2017
@jacogr jacogr merged commit d16ab5e into master Jan 5, 2017
@jacogr jacogr deleted the ng-decode-deployments branch January 5, 2017 11:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants