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

Contracts interface expansion #4307

Merged
merged 6 commits into from
Jan 26, 2017
Merged

Contracts interface expansion #4307

merged 6 commits into from
Jan 26, 2017

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Jan 26, 2017

  1. Modify contracts/Registry to have lookupMeta (interface to contract get) method
  2. Fixed contracts/GithubHint to retrieve instance properly & add getEntry to retrieve a specific entry

(Split from https://github.com/ethcore/parity/pull/4178)

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Jan 26, 2017
@jacogr jacogr mentioned this pull request Jan 26, 2017
this._instance = contract.instance;
return this._instance;
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implementation aligns with what we have in the others in ~/contracts now - previously it actually didn't work at all.

@@ -101,4 +101,17 @@ export default class Registry {
return address;
});
}

lookupMeta (_name, key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it make sense to now use this lookupMeta method in lookupAddress instead of having twice the same code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it that way around initially since it is 99.9% shared. Not sure why it was reverted, will take a re-look.

Copy link
Contributor Author

@jacogr jacogr Jan 26, 2017

Choose a reason for hiding this comment

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

I see why... one calls get the other getAddress on the underlying contract. (We don't have Uint in yet, haven't used it at this point in time)

So the options would be to pass the name, key as well as the relevant contract method to some helper to call the contract or to duplicate for code clarity.

I opted for the latter approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes good call. Well I guess that's fine, it small enough. Could have a

getLookupParams (_name, key) {
  const name = _name.toLowerCase();
  const sha3 = this._api.util.sha3.text(name);

  return [sha3, key];
}

method though

@ngotchac ngotchac added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 26, 2017
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. A0-pleasereview 🤓 Pull request needs code review. labels Jan 26, 2017
@jacogr jacogr merged commit b74f2d8 into master Jan 26, 2017
@jacogr jacogr deleted the jg-contracts branch January 26, 2017 16:51
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

2 participants