Exposed Deadline to the public in Transaction class #18
Conversation
Pull Request Test Coverage Report for Build 62
💛 - Coveralls |
There was a problem hiding this comment.
- Testing Missing
- It might be not the best approach, since when you receive a transaction from network you are able to change the
deadlineand then the Transaction Hash won't be the same.
I might opt for a method similar to replyGiven(deadline: Deadline): Transaction; which creates a new Transaction with the previous data, replacing the deadline and without TransactionInfo
|
implementation and test done. please review @aleixmorgadas |
package.json
Outdated
| @@ -5,6 +5,7 @@ | |||
| "scripts": { | |||
| "pretest": "npm run build", | |||
| "test": "mocha --ui bdd --recursive ./dist/test --timeout 90000", | |||
| "tran": "mocha --ui bdd --recursive ./dist/test/model/transaction/Transaction.spec --timeout 90000", | |||
There was a problem hiding this comment.
sorry, I was just testing and forgot to remove it :P it's faster because somehow in my console mocha couldn't filter this ./src/test/model/transaction/Transaction.spec
src/model/transaction/Transaction.ts
Outdated
| * @returns {Transaction} | ||
| * @memberof Transaction | ||
| */ | ||
| public reapplygiven(deadline: Deadline): Transaction { |
There was a problem hiding this comment.
replace reapplygiven by reply or replyGiven
There was a problem hiding this comment.
I would add the deadline as optional parameter, like
public reply(deadline: Deadline = Deadline.create()): Transaction
src/model/transaction/Transaction.ts
Outdated
| if (this.isUnannounced()) { | ||
| return Object.assign({__proto__: Object.getPrototypeOf(this)}, this, {deadline}); | ||
| } else { | ||
| throw new Error('an Announced transaction can\'t be modified'); |
There was a problem hiding this comment.
does exist a case where you want to reply an already announced transaction?
I would do:
if (this.isUnannounced()) {
return Object.assign({__proto__: Object.getPrototypeOf(this)}, this, {deadline});
}
throw new Error('an Announced transaction can\'t be modified');The code is more clean, personal style 😄
| @@ -98,6 +98,52 @@ describe('Transaction', () => { | |||
| expect(transaction.hasMissingSignatures()).to.be.equal(true); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('reapplygiven', () => { | |||
There was a problem hiding this comment.
updated according to the previous comments
| undefined, | ||
| ); | ||
|
|
||
| const newTransaction = transaction.reapplygiven(Deadline.create(3)); |
There was a problem hiding this comment.
I would move check that the new deadline is assigned, something like:
const newDeadline = Deadline.create(3);
const newTransaction = transaction.reapplygiven(newDeadline);
expect(newTransaction.deadline).to.be.equal(newDeadline);
ghost
merged commit 
No description provided.