[Applicaton] Provable past delegations & delayed undelegations #497
Conversation
red-0ne
added
application
|
@red-0ne Aiming to take a look at this tomorrow. |
|
Haven't started looking at the code yet but great PR summary @red-0ne! |
There was a problem hiding this comment.
@red-0ne Did a partial review (about 50% done) and it's a good start but I think this will require either iteration or discussion. Lmk what you prefer after seeing some of the comments.
- I don’t fully understand the logic in
pkg/crypto/rings/client.go. We either need to discuss or follow up async. - We added a lot of on-chain logic but there are 0 integration / unit tests. At least a few are necessary.
- There’s a lot of indexing / storage code that I don’t believe is needed. See my comment below.
When we discussed it on a call, I thought it would be as simple ass:
- Add
map[height]-> list[gateway_addr]to theApplicationproto. - Ensure
heightis always asession_end_heightinside theundelegate - Prune / update directly from the map
- New delegations mid session: active immediately
- Undelegations mid session: active until session ends
| @@ -0,0 +1,61 @@ | |||
| Feature: Delegate Namespaces | |||
|
|
|||
| Background: | |||
There was a problem hiding this comment.
TIL! Going to start using this.
There was a problem hiding this comment.
Ended up going through [1] to learn more but not going to change anything now.
| const ( | ||
| // serviceId to stake for by applications. | ||
| serviceId = "anvil" | ||
| // defaultStakeAmount is the default amount to stake for applications and gateways. |
There was a problem hiding this comment.
NIT: Can we decouple app & gateway stakes? It'll be more "scalable" from a dec perspective.
| } | ||
|
|
||
| // Fund the actor with enough upokt to stake for the service. | ||
| s.TheUserSendsUpoktFromAccountToAccount(int64(stakedAmount+1), "pnf", accName) |
There was a problem hiding this comment.
- Can you move
pnfinto a localfundingAccountvariable? - Can you try using
faucetinstead ofpnf?
| // Stake for the service with the | ||
| s.TheUserStakesAWithUpoktForServiceFromTheAccount( | ||
| accType, | ||
| int64(stakedAmount+1), |
There was a problem hiding this comment.
- Move
stakedAmount+1stakeAmount := stakedAmount + 1on line 41 - Reuse it in both places
- #PUC why we're doing +1
|
|
||
| Scenario: User can delegate Application to Gateway | ||
| Given the user has the pocketd binary installed | ||
| And the actor type "application" with account "app1" is staked with enough uPOKT |
There was a problem hiding this comment.
I looked at the implementation of is staked with enough uPOKT and I think it's actually is restaked with one additional uPOKT.
I don't fully understand the intention - yet - but there's an opportunity to make it clearer.
| @@ -92,6 +92,9 @@ func TestMsgServer_UndelegateFromGateway_SuccessfullyUndelegate(t *testing.T) { | |||
| _, err = srv.UndelegateFromGateway(ctx, undelegateMsg) | |||
There was a problem hiding this comment.
Coul we increase the unit and/or integration test coverage? We added A LOT of on-chain logic but zero new (non end to end tests) were added.
| // Undelegation is a message type used to schedule undelegating an application | ||
| // from a gateway. | ||
| message Undelegation { | ||
| string app_address = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; |
There was a problem hiding this comment.
Should we make this (gogoproto.nullable) = false]?
|
|
||
| // Undelegation is a message type used to schedule undelegating an application | ||
| // from a gateway. | ||
| message Undelegation { |
There was a problem hiding this comment.
Let's update the name. Undelegation is VERY general purpose in crypto.
UndelegationFromAppToGatewayEvent for example is better.
| gatewayIdx := slices.Index(foundApp.DelegateeGatewayAddresses, msg.GatewayAddress) | ||
| if gatewayIdx == -1 { | ||
| return nil, types.ErrAppNotDelegated.Wrapf( | ||
| "application not delegated to gateway with address %q", |
There was a problem hiding this comment.
| "application not delegated to gateway with address %q", | |
| "application cannot undelegated from gateway with address %q that it is not delegated to", |
| logger.Info(fmt.Sprintf("Application not found with address [%s]", msg.AppAddress)) | ||
| return nil, types.ErrAppNotFound.Wrapf("application not found with address: %s", msg.AppAddress) | ||
| return nil, types.ErrAppNotFound.Wrapf( | ||
| "application not found with address %q", |
There was a problem hiding this comment.
| "application not found with address %q", | |
| "application trying to undelegate with address %q not found", |
There was a problem hiding this comment.
@red-0ne Did a partial review (about 50% done) and it's a good start but I think this will require either iteration or discussion. Lmk what you prefer after seeing some of the comments.
- I don’t fully understand the logic in
pkg/crypto/rings/client.go. We either need to discuss or follow up async. - We added a lot of on-chain logic but there are 0 integration / unit tests. At least a few are necessary.
- There’s a lot of indexing / storage code that I don’t believe is needed. See my comment below.
When we discussed it on a call, I thought it would be as simple ass:
- Add
map[height]-> list[gateway_addr]to theApplicationproto. - Ensure
heightis always asession_end_heightinside theundelegate - Prune / update directly from the map
- New delegations mid session: active immediately
- Undelegations mid session: active until session ends
| // undelegations involve archiving old delegations records which, depending on | ||
| // the pruning parameters may result in the deletion of all archived records. | ||
|
|
||
| if err := k.EndBlockerProcessPendingUndelegations(ctx); err != nil { |
There was a problem hiding this comment.
| if err := k.EndBlockerProcessPendingUndelegations(ctx); err != nil { | |
| // Make sure to add comment here and below | |
| if err := k.EndBlockerProcessPendingUndelegations(ctx); err != nil { |
There was a problem hiding this comment.
Notes from offline discussion.
// Submit unelegation during sessionNUmber = 3
application.delegations = removeFromList(application.delegations, gatewayAddress) // remove from active
application.pendingUndelegation[sessionNumber=3] = addToList(application.pendingUndelegation[sessionNumber], gatewayAddress)
Rename the map: `mapOfSessionNumberToGatewayAddressThatWillBeUndelegatedFromInTheFutureuButMayStillBeUsedToValidateActiveSession`
// Verifying signature
// Assume verifiying signature at sessionNUmber = 5
// 1. Get ACTIVE gateway address
activeAddress = application.delegations
// 2. Get PENDING gateway address
// Assume session 3 is from block 10 to block 15
// If undelegation at block 11 -> session Number = 3 // filtered at session=3 -> returns addr
// If undelegation at block 12 -> session Number = 3 // filtered -> returns addr
// If undelegation at block 13 -> session Number = 3 // filtered -> returns addr
// If undelegation at block 14 -> session Number = 3 // filtered -> returns addr
// If undelegation at block 15 -> session Number = 3 // filtered -> returns addr
// If undelegation at block 16 -> session Number = 4 // filtered at session = 3 -> not return
filteredAddress = filtered(application.pendingUndelegation[sessionNumber=3], currentSessionNumber=5)
// filteredFunction: if sessionNumber < currentSessionNUmber: return; else don't return
// 3. Get all session addresses
all = active + filtered
// 4. Do normal business logic
|
Action Plan:
ClaimValidation() {
addressesToBuildRing = getActiveGatewayAddr(app, currentSessionNumber)
}
func getActiveGatewayAddr(app, currSessNumber) {
activeGateways = app.activeGateways
for sessionNumber, pendingUndelegations := range app.pendingUndelegations {
if sessionNumber < currSessNumber {
activeGateways = append(activeGateways, pendingUndelegations...)
}
}
return activeGateways
}
func EndBlocker() {
cleanUpPendingUndelegations(currSessionNumber)
}
func cleanUpPendingUndelegations() {
sessionNumbersToRemoveUndelegationsFor = someFunctionUsingGovParam(currSess, numSessionsToGoBack)
for app := allApplications {
for sess, undelegations := app.undelegations {
if sess <sessionNumbersToRemoveUndelegationsFor {
delete(app.undelegations, sess)
}
}
}
} |
|
Closing in favor of #518 |
Love two things about this:
💪 |
Summary
Add
EndBlockers keeper methods to:Update the
RingClientandRingCacheto use archived delegations when neededRemove stale code implementing
PubKeyClient2255 added lines of code are auto-generated pulsar files
A followup PR for e2e testing the feature involving sending relays to gateways instead of self-signingE2E tests for:AppGateServers will come after.Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist