Add assets, and exchanges API #7
Conversation
src/Collection/AssetsRecord.php
Outdated
| use ScriptFUSION\Porter\Collection\CountableProviderRecords; | ||
| use ScriptFUSION\Porter\Provider\Resource\ProviderResource; | ||
|
|
||
| class AssetsRecord extends CountableProviderRecords |
There was a problem hiding this comment.
This class and the ExchangesRecord class doesn't do anything and should be removed. Just invoke new CountableProviderRecords directly.
There was a problem hiding this comment.
Can you show some sample about invoking CountableProviderRecords class directly?
There was a problem hiding this comment.
I see that. Using the CountableProviderRecords directly because it has no any additional argument to be assigned.
| { | ||
| private $apiKey; | ||
|
|
||
| public function __construct(string $apiKey) |
There was a problem hiding this comment.
It is bad design to specify the API key in the resource because it must be duplicated into every resource. Specify it in the connector instead, to eliminate this duplication.
There was a problem hiding this comment.
Should I set the encrypted env variable in .travis.yml file?
Thanks.
There was a problem hiding this comment.
Ok, I also use the travis-encrypt tool to encrypt the env variable.
src/Resource/GetAssets.php
Outdated
| $response = \json_decode( | ||
| (string) $connector->fetch( | ||
| CryptoMonitor::buildExchangeApiUrl( | ||
| "v1/assets" |
There was a problem hiding this comment.
Double quotes not needed here. Prefer single quotes where interpolation not used to clearly express intent. Applies throughout this repository.
tests/Functional/GetAssetsTest.php
Outdated
| final class GetAssetsTest extends TestCase | ||
| { | ||
| /** @var $apiKey This is the Coin API Key for test environment*/ | ||
| private $apiKey = '4E861687-19D6-4894-87B9-E785B1EE3900'; |
There was a problem hiding this comment.
As mentioned before, you shouldn't be committing keys to your repository. They can be used by anyone, which is a security problem. Pass keys at invocation time via environment variables instead. This also fixes the duplication issue.
src/Resource/GetAssets.php
Outdated
|
|
||
| $assets = new \ArrayIterator($response); | ||
|
|
||
| return new AssetsRecord($assets, count($assets), $this); |
There was a problem hiding this comment.
Instantiate CountableProviderRecords here instead.
There was a problem hiding this comment.
I use the CountableProviderRecords directly and remove the AssetsRecord and ExchangesRecord. They're useless.
|
It has to be merged because all request changes should be done. |
@josantonius, please take a look at this.
I add the list all exchanges and list all assets.
@Bilge, if possible, please review this PR and suggest the recommendation.
And I think that's the proper way to implement these two lists API.
It's related to issue #2.
Thanks.