-
Notifications
You must be signed in to change notification settings - Fork 60
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
Get public keys from DepositEvent
#1957
Conversation
@@ -15,7 +15,6 @@ export function stakeBuilder(): IBuilder<Stake> { | |||
}) as `0x${string}`, | |||
) | |||
.with('state', faker.helpers.arrayElement(Object.values(StakeState))) | |||
.with('effective_balance', faker.string.numeric()) |
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.
effective_balance
was removed from the entity because it is not returned when a Stake
is not yet active, meaning it would fail validation. The value, in general, is also not used.
const isConfirmed = | ||
'confirmations' in transaction && | ||
!!transaction.confirmations && | ||
transaction.confirmations.length >= transaction.confirmationsRequired; |
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 general txStatus
signifies if a transaction is awaiting signatures/executions. The txInfo.status
of a staking transaction is inherently staking specific, e.g. NOT_STAKED
, DEPOSIT_IN_PROGRESS
, etc.
Pull Request Test Coverage Report for Build 11031317793Details
💛 - Coveralls |
@@ -13,6 +14,7 @@ const mockLoggingService = { | |||
warn: jest.fn(), | |||
} as jest.MockedObjectDeep<ILoggingService>; | |||
|
|||
// TODO: Move function encoding to kiln-encoder.builder.ts |
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.
This is covered in #1959, which I've marked as a draft until this is merged.
return []; | ||
} | ||
|
||
// There is only one DepositEvent per transaction |
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: Do you think it would be useful to emit a warning log line if this condition is not met for some reason?
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.
As the contracts are immutable, this should inherently never happen. However, on the off chance it does, warning makes sense. I added a warn
and a comment in 0fe91cd.
Instead of having multiple statuses we are now opting for one stake status. Aligning with the changes on cgw in this PR: safe-global/safe-client-gateway#1957
The base branch was changed.
Instead of having multiple statuses we are now opting for one stake status. Aligning with the changes on cgw in this PR: safe-global/safe-client-gateway#1957
Summary
In order to return the status of a given staking transaction, we fetch the
Stake
s from Kiln via validator keys. Generally, this is extracted from theDataDecoded
. However,deposit
transactions have no_publicKeys
parameter.This fetches the transaction status of a deposit transaction via it's transaction hash and, from it, decodes the public keys of the related validators. With that, the aforementioned status mapping is done.
Changes
KilnDecoder['decodeDepositEvent']
.deposit
transaction status.