Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contract selection with url params #2729

Closed
wants to merge 21 commits into from

Conversation

OlivierDemeaux
Copy link

Description

Added some code to allow a contract, address and abi to be set on the mtCrypto/contracts/interact page with a param passed in the URL. No need to search the contract in the dropdown.
ex: "myCrypto/contracts/interact?name=BCdiploma - Evidenz".
If a param is passed in the URL, the contract, address and abi will be set and the button "access" will be click.
PROBLEME: The access button crashes the localhost i'm running myCrypto on. (Even when i click on access button after i manually clicked on the contract)

Changes

  • Can now pass a contract name in the url as param to have it automatically selected on the page myCrypto/contracts/interact
    ex: "myCrypto//contracts/interact?name=BCdiploma%20-%20EvidenZ"

Steps to Test

  1. yarn install
  2. yarn start
  3. run url "localhost/contracts/interact?name=BCdiploma%20-%20EvidenZ" (works with every contract name)
  4. If localhost crashes, edit out the line 85, 86, 87 of the file : common/containers/Tabs/Contracts/components/Interact/components/InteractForm/index.tsx.
    Those lines are responsibles for the click on the "access" button after that the contract name, address and abi are set.

Zeplin Design

Quality Assurance

  • [✓] The branch name is in lowercase-kebab-case with no prefix (unless it was created from Clubhouse)
  • [✓] The base branch is develop or gau (no nested branches)
  • [✓] This is related to a maximum of one Clubhouse story or GitHub issue
  • [✓] Types are safe (avoid TypeScript/TSLint features like any and disable, instead use more specific types)
  • [✓] If code is copied from existing directories, there is an explanation of why this is necesary in the description/changes, and all copying is done in separate commits to make them easy to filter out

@mycrypto-bot
Copy link
Collaborator

Staging build: MyCryptoBuilds

contracts.map(con => {
const addr = con.address ? `(${con.address.substr(0, 10)}...)` : '';
// if contract name from Url exists, then load this contract
if (con.name === contractName) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make a modification here to do the comparison as both strings in lowercase for a UX factor (ie: MyContract contract name won't match mycontract query string value)

Copy link
Author

Choose a reason for hiding this comment

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

Good idea! Thanks. Did just that. Still not working on a live version even tho it works on a localhost so making a bunch of console.log to understand what's happing

@coveralls
Copy link

coveralls commented Sep 2, 2019

Coverage Status

Coverage remained the same at 53.035% when pulling f54c09b on OlivierDemeaux:develop into 7ca14a9 on MyCryptoHQ:develop.

@blurpesec blurpesec self-requested a review September 7, 2019 01:06
@mwbailey
Copy link
Contributor

mwbailey commented Sep 9, 2019

Hi @OlivierDemeaux. I got curious when I saw you mention that strange localhost vs non-localhost bug on the MyCrypto Discord. As you've discovered, the problem is the undefined abi field in the Contract class in common/libs/contracts/index.ts, which you fixed by assigning it a value in the Interact component.

Turns out the reason it was crashing the development build but not the production build wasn't because of a localhost issue. The development build (when you run yarn start) uses Babel to compile/transpile the Typescript, and the production build (e.g., yarn build) uses the official Typescript compiler (tsc). I assume the reason they set it up this way is that Babel has faster build times and React Hot Loader compatibility, but tsc is needed to actually do type-checking.

Babel (correctly, according to the proposed spec) transpiles the uninitialized abi field as though it were abi = undefined. This results in the crash in development builds. The Typescript compiler currently ignores uninitialized field declarations, so the abi property does not appear in the emitted Javascript code at all. That's why the production build doesn't crash when iterating through the properties in Contract.getFunctions().

Unless I'm missing something, it seems like the public abi: any; line in the Contract class can just be deleted. The production build has been running without it with no apparent problems. I don't see anywhere in the code where that abi field is used. (Note: there's a separate NetworkContract interface with an abi field that is needed.) I think it may be an unnecessary leftover from a refactor of the Contract code about 2 years ago around release tag 0.0.6.

@blurpesec blurpesec added the status: changes needed Open PR's if changes are needed after technical or QA review. label Sep 11, 2019
}
};

public getParamsFromUrl = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function seems to be repeated 3x. Is there any way that you can extract this out, or use it in a higher order component and pass the data down as props?

Copy link
Author

Choose a reason for hiding this comment

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

Took me longer than i want to admit but I managed to extract it! Tell me what you think about it!

const item = params[index];
if (index.substr(0, 5) === 'param') {
count++;
const element: any = document.getElementById(index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain what this method is attempting to do and why this getElementById is needed?

Copy link
Author

Choose a reason for hiding this comment

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

Just grab the params from the url and set them in the function's input fields according to their respective numbers. It needs getElementById in order to get the input and set the value to that input

Copy link
Author

Choose a reason for hiding this comment

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

Hi @blurpesec. Any news?

@VinceBCD
Copy link
Contributor

Hi guys! From our point of view, it's a must have for adoption. If you have time I can give you a demo of our product to show you how crucial it is (and not only for ourselves.)
With this feature, the general public can, in an application context, verify information stored on the blockchain with a single click. This PR is opened for 3 months now, how to move forward ?

@FrederikBolding
Copy link
Collaborator

Closing in favor of #2920

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes needed Open PR's if changes are needed after technical or QA review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants