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

Refactor Transaction Type #37

Open
Techworker opened this issue Jan 22, 2018 · 4 comments
Open

Refactor Transaction Type #37

Techworker opened this issue Jan 22, 2018 · 4 comments

Comments

@Techworker
Copy link
Member

The Techworker\IOTA\Type\Transaction class has way too much logic embedded. We will need to refactor most parts of it, especially the parse and __toString methods contain a whole bunch of logic to either interprete or create a transaction trytes string.

Together with that, we should think about something like lazy parsing. The parsing process itself is not complicated but really slow, so initializing a Transaction instance with existing Trytes takes much too long.

@peter279k
Copy link
Contributor

This is not the refactoring work. Consider the following code snippets in __toString method:

$valueTrits = TritsUtil::fromInt($this->value->getAmount(), 81);
while (\count($valueTrits) < 81) {
    $valueTrits[] = 0;
}

This will be the infinite loop and it's the JavaScipt "feature". It can see more details about this.

This work should be existed because the TritsUtil::toTrytes method will check whether the trits array length is the multiple of 3 then do some works.

I think it's the bug.

@peter279k
Copy link
Contributor

peter279k commented Aug 24, 2018

@Techworker .After reviewing the parse method, most of codes don't have the problems. I only have the one optional problem is as follows:

  • The substring usage in JavaScript is different from substr usage in PHP.

  • The substring usage in JavaScript uses the start index and last index and in the PHP uses the start index and subtracted string length.

I know you want to compare these two sub string function behaviors so it lets the two numbers do minus calculation.

I think this is the optional refactoring work: Let the second parameter be the current subtracted length number without the calculating work.

What do you think?

@Techworker
Copy link
Member Author

There was a simple reason why it looks like that. The old iota.lib.js looked like this: https://github.com/iotaledger/iota.js/blob/4735f0c5a39d73f6fa43de51d4334278347e5ffb/dist/iota.js#L4181-L4192

And I wanted to make it simpler to compare both versions in case something changes. So instead of, for instance, using

$this->address = new Address(substr($this->trytes, 2187, 81));

..I used..

$this->address = new Address(substr($this->trytes, 2187, 2268 - 2187));

..to make sure it adheres with the numbers from the JS implementation. It's just a readability and compatibility issue, but you can change it if you want to. The new typescript iota lib now defined constants for the lengths: See here

The main problem with this class is that it takes a huge amount of time to initialize an existing transaction, you should try it and you'll see what I mean.

https://gist.github.com/Techworker/2aefb51dfc1041dc78f8af1082a8878e

This takes like 0.4 seconds on my laptop with PHP 7.2, but I think this is related to Curl.

@peter279k
Copy link
Contributor

peter279k commented Aug 26, 2018

@Techworker, thank you for your reply.

How about letting the subtracted length be the constraint value?

Do you have any idea about defining this constraint variable name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants