Transaction signer, callable SmartContracts, Update user connector. #21
Conversation
…ereum_txsigner/js/metamask/src/mascara/index.html
|
@amateescu Do I use the services wisely now? I'm sometimes a little unsure. Ii would be very happy to get your feedback on that. |
| * @return array | ||
| * Table render array. | ||
| */ | ||
| public function getDeployedAsTable() { |
There was a problem hiding this comment.
An entity object shouldn't provide methods that output render arrays, that's the job of a list builder or a specialized controller.
There was a problem hiding this comment.
Moved to ethereum_user_connector/src/Form/AdminForm.php
| /** | ||
| * @inheritdoc | ||
| */ | ||
| public function status(); |
There was a problem hiding this comment.
This method is already defined by ConfigEntityInterface so we don't need to declare it again.
| * | ||
| * @var boolean | ||
| */ | ||
| protected $status; |
There was a problem hiding this comment.
This is already defined by ConfigEntityBase, no need to redefine it here.
| @@ -21,15 +23,20 @@ class EthereumController extends ControllerBase { | |||
| * | |||
| * @var \Ethereum\Ethereum | |||
| */ | |||
| protected $client; | |||
| public $client; | |||
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
Made protected and renamed to web3 for consistency.
| */ | ||
| public function __construct(Ethereum $ethereum_client) { | ||
| $this->client = $ethereum_client; | ||
| public function __construct(Ethereum $web3 = NULL) { |
There was a problem hiding this comment.
This change should not be needed since we're are injecting the ethereum client from the container.
I assume that some other code is not initializing this class properly..
There was a problem hiding this comment.
Hopefully fixed. This topic is still confusing for me.
src/Entity/EthereumServer.php
Outdated
| $client = \Drupal::service('ethereum.client_factory')->get($this->getUrl()); | ||
| /** @var \Ethereum\Ethereum $web3 */ | ||
| $web3 = new Ethereum($this->url); | ||
| // @todo How to load the \Drupal::service('ethereum.client') with a updated url? |
There was a problem hiding this comment.
The previous code was already handling that..
There was a problem hiding this comment.
ok. Reverted to your code.
src/Entity/EthereumServer.php
Outdated
| * @return array | ||
| * Table render array. | ||
| */ | ||
| public function getServerInfoAsTable() { |
There was a problem hiding this comment.
As mentioned above, we shouldn't put these kinds of methods on the config entity, but in their specific controller instead.
src/Form/EthereumServerForm.php
Outdated
| if ($status == SAVED_UPDATED) { | ||
| $this->messenger()->addStatus($this->t('The server %label has been updated.', ['%label' => $server->label()])); | ||
| drupal_set_message($this->t('The server %label has been updated.', ['%label' => $server->label()])); |
There was a problem hiding this comment.
drupal_set_message() is deprecated, the previous code is using the proper API.
There was a problem hiding this comment.
cleaned up all usages of drupal_set_message()
src/Form/EthereumServerForm.php
Outdated
| } | ||
| elseif ($status == SAVED_NEW) { | ||
| $this->messenger()->addStatus($this->t('The server %label has been added.', ['%label' => $server->label()])); | ||
| drupal_set_message($this->t('The server %label has been added.', ['%label' => $server->label()])); |
There was a problem hiding this comment.
cleaned up all usages of drupal_set_message()
| @@ -35,6 +35,10 @@ public static function defaultSettings() { | |||
| public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) { | |||
| $element = parent::formElement($items, $delta, $element, $form, $form_state); | |||
|
|
|||
| if ($element['value']['#default_value']) { | |||
| $element['value']['#attributes']['data-original-value'] = $element['value']['#default_value']; | |||
There was a problem hiding this comment.
Why do we need the original value here?
There was a problem hiding this comment.
I use it to check if a address entry is the same than before, in order to give the address_status the right value. You are right: technically it should be somewhere in ethereum_user_connector module. There are 3 fields on the user required to manage the contract registry subscription (address, hash, address_status).
| $eth = new EthereumUserConnectorController(); | ||
| $validation = $eth->verifyUserByHash(Xss::filter($hash)); | ||
|
|
||
| $controller = new EthereumUserConnectorController(); |
There was a problem hiding this comment.
Here and in all the other places which instantiate EthereumUserConnectorController, we need to do it through the class resolver instead, like this: \Drupal::service('class_resolver')->getInstanceFromDefinition(EthereumUserConnectorController::class)
This will properly inject the dependencies from the container, and I'm pretty sure it's the reason for the changes to the constructor of \Drupal\ethereum\Controller\EthereumController (i.e. those changes can be reverted)
| @@ -0,0 +1,92 @@ | |||
| # Metamask Mascara wrapper | |||
There was a problem hiding this comment.
Shouldn't this be a stand-alone JS library (in a separate github repo) that the module would require?
There was a problem hiding this comment.
Yes. It actually is a external library.
What I still miss is how to load external libs with composer. Seems we need to add asset-packagist to the root composer json like Thunder distribution does. Was hoping we will get that in core soon, before externalizing the dependencies.
No description provided.