Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

[feat] add transactionId property #55

Merged
merged 2 commits into from
Aug 29, 2018
Merged

[feat] add transactionId property #55

merged 2 commits into from
Aug 29, 2018

Conversation

n1c01a5
Copy link
Contributor

@n1c01a5 n1c01a5 commented Aug 18, 2018

No description provided.

@n1c01a5 n1c01a5 requested a review from satello August 18, 2018 09:27
@n1c01a5 n1c01a5 mentioned this pull request Aug 18, 2018
Closed
satello
satello suggested changes Aug 20, 2018
View reviewed changes
Copy link
Contributor

@satello satello left a comment

Choose a reason for hiding this comment

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

Is transactionId the tx hash? I prefer the variable txHash. Notifications already use txHash so better to keep it consistent

@n1c01a5
Copy link
Contributor Author

n1c01a5 commented Aug 21, 2018 •
edited
Loading

I agree it's a little confusing but the in general I prefer follow the smart contract wording and the associated method is createTransaction https://github.com/kleros/kleros-interaction/blob/68701d844ea86d12d0a1aa8db0fa2aa7ae79ca1d/contracts/standard/arbitration/MultipleArbitrableTransaction.sol#L224 so I prefer used transactionId to be consistent with the smart contract

@n1c01a5 n1c01a5 merged commit 08c8482 into develop Aug 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@satello satello satello approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@n1c01a5 @satello