-
Notifications
You must be signed in to change notification settings - Fork 9
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
implement wait() method for QiTx response #317
implement wait() method for QiTx response #317
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wait mechanism looks good to go.
I would like to avoid changing any of the interfaces that force passing in unnecessary inputs like hash and zone when they are not needed. I know that this was done because Qi transactions where not producing correct QIP10 hashes and this was the only way to get this to work but we need to test again against latest go-quai and remove whatever is not needed.
src/providers/abstract-provider.ts
Outdated
} | ||
case 'qiTransaction': { | ||
assert(zone != null, 'zone is required for qiTransaction event', 'MISSING_ARGUMENT'); | ||
assert(hash != null, 'hash is required for qiTransaction event', 'MISSING_ARGUMENT'); | ||
return { type: _event, tag: _event, hash, zone }; | ||
} | ||
} | ||
} | ||
|
||
if (isHexString(_event, 32)) { | ||
const hash = _event.toLowerCase(); | ||
zone = toZone(hash.slice(0, 4)); | ||
zone = zone ?? toZone(hash.slice(0, 4)); | ||
return { type: 'transaction', tag: getTag('tx', { hash }), hash, zone }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we add an extra hash argument to the getSubscription method if we are already handling hash events for regular transactions? Cant we just use that same pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have re implemented the Qi Tx wait() logic under the assumption that the tx input hash is permuted following QIP10.
src/providers/abstract-provider.ts
Outdated
@@ -1674,8 +1686,8 @@ export class AbstractProvider<C = FetchRequest> implements Provider { | |||
return this._wrapBlock(params, network); | |||
} | |||
|
|||
async getTransaction(hash: string): Promise<null | TransactionResponse> { | |||
const zone = toZone(this.shardFromHash(hash)); | |||
async getTransaction(hash: string, _zone?: Zone): Promise<null | TransactionResponse> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's revisit this once go-quai and the genallocs get updated. I believe the core team is going to enforce QIP10 hashes so we should be able to rely on zone derived from hash and drop the zone input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have re implemented the Qi Tx wait() logic under the assumption that the tx input hash is permuted following QIP10.
5be3d89
to
095346e
Compare
No description provided.