-
Notifications
You must be signed in to change notification settings - Fork 13
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
[Tokenomics] SettleSessionAccounting first implementation (emissions=burn) #323
Merged
Olshansk
merged 68 commits into
main
from
issues/243/settle_session_accounting_implementation
Feb 2, 2024
Merged
Changes from 59 commits
Commits
Show all changes
68 commits
Select commit
Hold shift + click to select a range
6f32dac
move protox to the right place
Olshansk 1d4e475
Merge branch 'main' into issues/243/update_tokenomics_module
Olshansk 86cca4f
Fix proto imports
Olshansk f317d89
Merge branch 'main' into issues/243/update_tokenomics_module
Olshansk 2c44d26
Documented the plan for this PR before I forget
Olshansk 4b1d025
Merge branch 'main' into issues/243/update_tokenomics_module
Olshansk f2519ea
Merge branch 'main' into issues/243/update_tokenomics_module
Olshansk c8b4b6b
Merge branch 'main' into issues/243/update_tokenomics_module
Olshansk 6364101
Update deps
Olshansk 9620a24
Unit test for updating tokenomic params succeeds
Olshansk 5b4949d
Added a placeholder for SettleSessionAccounting
Olshansk 565195b
Rename Compute to ComputeUnits
Olshansk ee71f7d
Remove some unused comments
Olshansk 7fe5ecb
Before scaffold
Olshansk 0079a32
Ran the following command to fill in some missing gaps: ignite scaffo…
Olshansk 581c947
Merge branch 'main' into issues/243/update_tokenomics_module
Olshansk 7939b4c
Incomplete unit tests for parameter update passing
Olshansk 1c32614
Checkpoint for the day
Olshansk fa80df8
Merge with main
Olshansk ee787d0
Update /msg_server_update_params_test.go and figurign some stuff out …
Olshansk 4aca865
Unit tests passing
Olshansk fe9a645
Remove session accounting implementation
Olshansk 4176971
Add session accounting implementation
Olshansk e32bc93
Completed self review
Olshansk ebf5a79
Merge branch 'issues/243/update_tokenomics_module' into issues/243/se…
Olshansk 7d7cc8e
Unit tests in progress
Olshansk 398e4d6
Update x/tokenomics/client/cli/tx_update_params.go
Olshansk cab8bee
change k8s DNS endpoint for celestia secret (#314)
okdas d95e1f7
[LocalNet] Add sequencer block persistence (#277)
okdas fd9662c
[Config, Docs] Add supplier staking config documentation (#318)
red-0ne 4eeda85
[Docs] Add AppGateServer config documentation (#317)
red-0ne 6d566eb
[Config, Docs] Add gateway staking config documentation (#321)
red-0ne eda0816
[Config, Docs] Add application staking config documentation (#320)
red-0ne 629bf6c
Fixed some tests
Olshansk 9a79163
Update proto/pocket/tokenomics/query.proto
Olshansk fa45776
Update Makefile
Olshansk f149a74
Update x/tokenomics/client/cli/tx_update_params.go
Olshansk 4b8a5b5
Update x/tokenomics/keeper/keeper.go
Olshansk de19c54
Reply to some review comments
Olshansk 1871007
Ran make go_imports and make go_lint
Olshansk 526d90e
Merge with main
Olshansk bdbbcc8
Start multi-lining
Olshansk d1fcf9b
Renamed some tokenomics errors
Olshansk 23fb106
Reply to PR review comments
Olshansk 7972eef
foundation/pnf
Olshansk 3f92915
Ran make go_imports and make go_lint
Olshansk d708df0
Fix outdated test
Olshansk 90b5255
Merge with main
Olshansk 9faf89c
Fix log usage
Olshansk 05a8924
Update Makefile
Olshansk 70eea62
fix: add yarn.lock (#334)
bryanchriswhite 4b92864
[Off-chain] Simplify `TxClient` with `EventsQueryClient` (#330)
bryanchriswhite 3e447db
Merge branch 'main' into issues/243/update_tokenomics_module
Olshansk 3b74750
Merge with issues/243/update_tokenomics_module
Olshansk e980a64
Added x/session/types/session_header_test.go
Olshansk 691f236
Unit tests passing
Olshansk a92ce85
Merge with master - not compiling yet
Olshansk db43050
Merge branch 'main' into issues/243/settle_session_accounting_impleme…
Olshansk 5b90116
Reply to some review comments
Olshansk 1d11fa2
Make go_test works again
Olshansk abe5173
Enforce smstRootSize
Olshansk aab2413
Merge branch 'main' into issues/243/settle_session_accounting_impleme…
Olshansk e1c8913
Update go.mod
Olshansk 3082237
Merge branch 'main' into issues/243/settle_session_accounting_impleme…
Olshansk 40a21f2
Added a WIP E2E test
Olshansk 31aa811
Merge branch 'main' into issues/243/settle_session_accounting_impleme…
Olshansk ba4d76b
Update a couple comments
Olshansk 6697a09
Merge with main and reply to some comments
Olshansk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
package types | ||
|
||
import ( | ||
sdkerrors "cosmossdk.io/errors" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
) | ||
|
||
// TODO_TECHDEBT: Make sure this is used everywhere we validate components | ||
// of the session header. | ||
func (sh *SessionHeader) ValidateBasic() error { | ||
// Validate the application address | ||
if _, err := sdk.AccAddressFromBech32(sh.ApplicationAddress); err != nil { | ||
return sdkerrors.Wrapf(ErrSessionInvalidAppAddress, "invalid application address: %s; (%v)", sh.ApplicationAddress, err) | ||
Olshansk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Validate the session ID | ||
// TODO_TECHDEBT: Introduce a `SessionId#ValidateBasic` method. | ||
if sh.SessionId == "" { | ||
return sdkerrors.Wrapf(ErrSessionInvalidSessionId, "invalid session ID: %s", sh.SessionId) | ||
} | ||
|
||
// Validate the service | ||
// TODO_TECHDEBT: Introduce a `Service#ValidateBasic` method. | ||
if sh.Service == nil { | ||
return sdkerrors.Wrapf(ErrSessionInvalidService, "invalid service: %s", sh.Service) | ||
} | ||
|
||
// Check if session end height is greater than session start height | ||
if sh.SessionEndBlockHeight <= sh.SessionStartBlockHeight { | ||
return sdkerrors.Wrapf(ErrSessionInvalidBlockHeight, "session end block height cannot be less than or equal to session start block height") | ||
} | ||
|
||
return nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
package types_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/pokt-network/poktroll/testutil/sample" | ||
"github.com/pokt-network/poktroll/x/session/types" | ||
sharedtypes "github.com/pokt-network/poktroll/x/shared/types" | ||
) | ||
|
||
func TestSessionHeader_ValidateBasic(t *testing.T) { | ||
tests := []struct { | ||
desc string | ||
sh types.SessionHeader | ||
err error | ||
}{ | ||
{ | ||
desc: "invalid - invalid application address", | ||
sh: types.SessionHeader{ | ||
ApplicationAddress: "invalid_address", | ||
SessionId: "valid_session_id", | ||
Service: &sharedtypes.Service{}, | ||
SessionStartBlockHeight: 100, | ||
SessionEndBlockHeight: 101, | ||
}, | ||
err: types.ErrSessionInvalidAppAddress, | ||
}, | ||
{ | ||
desc: "invalid - empty session id", | ||
sh: types.SessionHeader{ | ||
ApplicationAddress: sample.AccAddress(), | ||
SessionId: "", | ||
Service: &sharedtypes.Service{}, | ||
SessionStartBlockHeight: 100, | ||
SessionEndBlockHeight: 101, | ||
}, | ||
err: types.ErrSessionInvalidSessionId, | ||
}, | ||
{ | ||
desc: "invalid - nil service", | ||
sh: types.SessionHeader{ | ||
ApplicationAddress: sample.AccAddress(), | ||
SessionId: "valid_session_id", | ||
Service: nil, | ||
SessionStartBlockHeight: 100, | ||
SessionEndBlockHeight: 101, | ||
}, | ||
err: types.ErrSessionInvalidService, | ||
}, | ||
{ | ||
desc: "invalid - start block height greater than end block height", | ||
sh: types.SessionHeader{ | ||
ApplicationAddress: sample.AccAddress(), | ||
SessionId: "valid_session_id", | ||
Service: &sharedtypes.Service{}, | ||
SessionStartBlockHeight: 100, | ||
SessionEndBlockHeight: 99, | ||
}, | ||
err: types.ErrSessionInvalidBlockHeight, | ||
}, | ||
{ | ||
desc: "valid", | ||
sh: types.SessionHeader{ | ||
ApplicationAddress: sample.AccAddress(), | ||
SessionId: "valid_session_id", | ||
Service: &sharedtypes.Service{}, | ||
SessionStartBlockHeight: 100, | ||
SessionEndBlockHeight: 101, | ||
}, | ||
err: nil, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.desc, func(t *testing.T) { | ||
err := tt.sh.ValidateBasic() | ||
if tt.err != nil { | ||
require.ErrorIs(t, err, tt.err) | ||
} else { | ||
require.NoError(t, err) | ||
} | ||
}) | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 think referencing:
So you can control applications/suppliers in tests and modify the methods with custom
DoAndReturn
methods will help you better catch some errors. Maybe not applicable here, just a thought.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.
tl;dr Thought about it but do not believe it is worth the tradeoff in this chase.
cc @bryanchriswhite for additional thoughts or pushback.
I can steelman both sides here.
Firstly, I always bias to writing less code wherever possible: less code => less bugs => less to maintain => less to learn => more time on other things.
With that said, Complex state management in mocks is nice when you're testing some business logic, but in this specific instance, I feel that:
It's a very delicate balance and don't think there's a definitive answer to when to do which one.