-
Notifications
You must be signed in to change notification settings - Fork 19
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
revert: stalehash extension #2184
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.
nit: assuming there is still stalehash check in extrinsics?
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.
That is correct, we didn't remove that check.
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.
one comment, lgtm
Codecov ReportAll modified and coverable lines are covered by tests ✅ |
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.
Not sure it's the issue, but we'll revert just in case
@@ -232,22 +232,6 @@ describe('📗 Stateful Pallet Storage', function () { | |||
section: 'statefulStorage', | |||
}); | |||
}); | |||
|
|||
it('🛑 should fail call to remove page for un-delegated attempts', async function () { |
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.
These tests are not correct and I had to remove since we are using a badMsaId
which it's nonce is lower than current nonce for providerKeys it is throwing following error.
1010: Invalid Transaction: Transaction is outdated
which might be a nonce check race condition issue which is not related to this specific pallet or extrinsic
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.
On a second thought it is weird why this gets triggered
The following is the only place we are returning InvalidTransaction::Stale
.
// The default account (no account) has a nonce of 0.
// If account nonce is not equal to the tx nonce (self.0), the tx is invalid. Therefore, check if it is a stale or future tx.
if self.0 != account.nonce {
return Err(if self.0 < account.nonce {
InvalidTransaction::Stale
} else {
InvalidTransaction::Future
}
.into())
}
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 don't think it's a blocker for this PR merge but we should take a deeper look
Goal
The goal of this PR is
Closes #2183
Reverting #2137
Checklist