Skip to content
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

[Utility] Feat: Chain-specific compute unit to tokens multipliers #552

Merged
merged 20 commits into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f1194ec
Added ComputeUnitsPerRelay to Services
rBurgett Jun 10, 2024
4491832
Updated tests to not need compute units per relay fallback when settl…
rBurgett Jun 10, 2024
37555c7
Moved DefaultComputeUnitsPerRelay to service types
rBurgett Jun 10, 2024
3b32119
Updated comments, added compute units per relay tests, cleaned up tests
rBurgett Jun 12, 2024
826404f
Merge branch 'main' into issues/494/per-chain_cuttm
rBurgett Jun 12, 2024
dabe8f1
Updated fixture generator
rBurgett Jun 12, 2024
dcb6fbe
Updated comments
rBurgett Jun 15, 2024
5e9c3b8
Merge branch 'main' into issues/494/per-chain_cuttm
rBurgett Jun 15, 2024
cd2fef0
Documentation updates
rBurgett Jun 19, 2024
517ad39
Merge branch 'main' into issues/494/per-chain_cuttm
rBurgett Jul 2, 2024
753f79a
Update claim and e2e tests to properly handle relay to tokens multipl…
rBurgett Jul 8, 2024
0d5dbce
Merge branch 'main' into issues/494/per-chain_cuttm
rBurgett Jul 8, 2024
0228131
Updated settlement e2e test to make sure compute units per relay is s…
rBurgett Jul 8, 2024
717843d
Merge branch 'main' into issues/494/per-chain_cuttm
rBurgett Jul 16, 2024
f7559ca
Added service configs to applications in session accounting tests
rBurgett Jul 16, 2024
1305ed6
Cleaned up SettleSessionAccounting, updated BaseClaim parameters, add…
rBurgett Jul 23, 2024
a227d74
Merge branch 'main' into issues/494/per-chain_cuttm
rBurgett Jul 23, 2024
af04fe2
Fixed merge mistakes in SettleSessionAccounting
rBurgett Jul 23, 2024
0a0d786
Added ToDo in tx_add_service
rBurgett Jul 23, 2024
12ea443
Empty commit
rBurgett Jul 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 113 additions & 53 deletions api/poktroll/shared/service.pulsar.go

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions proto/poktroll/shared/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ message Service {

// TODO_BETA: Name is currently unused but acts as a reminder that an optional onchain representation of the service is necessary
string name = 2; // (Optional) Semantic human readable name for the service

// Used alongside the global 'compute_units_to_tokens_multipler' to calculate the cost of a relay for this service
uint64 compute_units_per_relay = 3; // Compute units required per relay for this service
}

// ApplicationServiceConfig holds the service configuration the application stakes for
Expand Down
9 changes: 5 additions & 4 deletions testutil/proof/fixture_generators.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,11 @@ func BaseClaim(appAddr, supplierAddr string, sum uint64) prooftypes.Claim {
SupplierAddress: supplierAddr,
SessionHeader: &sessiontypes.SessionHeader{
ApplicationAddress: appAddr,
Service: &sharedtypes.Service{
Id: "svc1",
Name: "svcName1",
},
Service: sharedtypes.NewService(
"svc1",
"svcName1",
1,
),
SessionId: "session_id",
SessionStartBlockHeight: 1,
SessionEndBlockHeight: testsession.GetSessionEndHeightWithDefaultParams(1),
Expand Down
21 changes: 17 additions & 4 deletions x/service/keeper/msg_server_add_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ func TestMsgServer_AddService(t *testing.T) {

// Declare test services
svc1 := sharedtypes.Service{
Id: "svc1",
Name: "service 1",
Id: "svc1",
Name: "service 1",
ComputeUnitsPerRelay: 1,
}

preExistingService := sharedtypes.Service{
Id: "svc2",
Name: "service 2",
Id: "svc2",
Name: "service 2",
ComputeUnitsPerRelay: 1,
}

// Generate a valid address
Expand Down Expand Up @@ -125,6 +127,17 @@ func TestMsgServer_AddService(t *testing.T) {
},
expectedErr: types.ErrServiceMissingName,
},
{
desc: "invalid - zero compute units per relay",
setup: func(t *testing.T) {},
address: sample.AccAddress(),
service: sharedtypes.Service{
Id: "svc1",
Name: "service 1",
ComputeUnitsPerRelay: 0,
},
expectedErr: types.ErrServiceInvalidComputUnitsPerRelay,
},
{
desc: "invalid - service already exists (same service supplier)",
setup: func(t *testing.T) {},
Expand Down
17 changes: 15 additions & 2 deletions x/service/module/tx_add_service.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package service
Olshansk marked this conversation as resolved.
Show resolved Hide resolved

import (
Olshansk marked this conversation as resolved.
Show resolved Hide resolved
"fmt"
"strconv"

"github.com/cosmos/cosmos-sdk/client"
Expand All @@ -15,14 +16,14 @@ var _ = strconv.Itoa(0)

func CmdAddService() *cobra.Command {
Olshansk marked this conversation as resolved.
Show resolved Hide resolved
cmd := &cobra.Command{
Use: "add-service <service_id> <service_name>",
Use: fmt.Sprintf("add-service <service_id> <service_name> [compute_units_per_relay: default={%d}]", types.DefaultComputeUnitsPerRelay),
Short: "Add a new service to the network",
Long: `Add a new service to the network that will be available for applications,
gateways and suppliers to use. The service id MUST be unique but the service name doesn't have to be.

Example:
$ poktrolld tx service add-service "svc1" "service_one" --keyring-backend test --from $(SUPPLIER) --node $(POCKET_NODE) --home $(POKTROLLD_HOME)`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the example w/ compute units per relay.

Args: cobra.ExactArgs(2),
Args: cobra.MinimumNArgs(2),
RunE: func(cmd *cobra.Command, args []string) (err error) {
serviceIdStr := args[0]
serviceNameStr := args[1]
Expand All @@ -32,10 +33,22 @@ $ poktrolld tx service add-service "svc1" "service_one" --keyring-backend test -
return err
}

computeUnitsPerRelay := types.DefaultComputeUnitsPerRelay
// if compute units per relay argument is provided
if len(args) > 2 {
computeUnitsPerRelay, err = strconv.ParseUint(args[2], 10, 64)
if err != nil {
return types.ErrServiceInvalidComputUnitsPerRelay.Wrapf("unable to parse as uint64: %s", args[2])
}
} else {
fmt.Printf("Using default compute_units_per_relay: %d\n", types.DefaultComputeUnitsPerRelay)
}

msg := types.NewMsgAddService(
clientCtx.GetFromAddress().String(),
serviceIdStr,
serviceNameStr,
computeUnitsPerRelay,
)
if err := msg.ValidateBasic(); err != nil {
return err
Expand Down
31 changes: 24 additions & 7 deletions x/service/module/tx_add_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package service_test

import (
"fmt"
"strconv"
"testing"

sdkerrors "cosmossdk.io/errors"
Expand Down Expand Up @@ -52,12 +53,14 @@ func TestCLI_AddService(t *testing.T) {

// Prepare two valid services
svc1 := sharedtypes.Service{
Id: "svc1",
Name: "service name",
Id: "svc1",
Name: "service name",
ComputeUnitsPerRelay: 1,
}
svc2 := sharedtypes.Service{
Id: "svc2",
Name: "service name 2",
Id: "svc2",
Name: "service name 2",
ComputeUnitsPerRelay: 1,
}
// Add svc2 to the network
args := []string{
Expand All @@ -81,6 +84,15 @@ func TestCLI_AddService(t *testing.T) {
supplierAddress: account.Address.String(),
service: svc1,
},
{
desc: "valid - add new service without compute units per relay",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we make this invalid?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at the coce.

Please update test description to explain that it's valid because it defaults to the global value.

supplierAddress: account.Address.String(),
service: sharedtypes.Service{
Id: svc1.Id,
Name: svc1.Name,
ComputeUnitsPerRelay: 0,
},
},
{
desc: "invalid - missing service id",
supplierAddress: account.Address.String(),
Expand Down Expand Up @@ -114,12 +126,17 @@ func TestCLI_AddService(t *testing.T) {
require.NoError(t, net.WaitForNextBlock())

// Prepare the arguments for the CLI command
args := []string{
argsAndFlags := []string{
test.service.Id,
test.service.Name,
fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress),
}
args = append(args, commonArgs...)
if test.service.ComputeUnitsPerRelay > 0 {
// Only include compute units per relay argument if provided
argsAndFlags = append(argsAndFlags, strconv.FormatUint(test.service.ComputeUnitsPerRelay, 10))
}
argsAndFlags = append(argsAndFlags, fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress))

args := append(argsAndFlags, commonArgs...)

// Execute the command
addServiceOutput, err := clitestutil.ExecTestCLICmd(ctx, service.CmdAddService(), args)
Expand Down
25 changes: 13 additions & 12 deletions x/service/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,17 @@ import sdkerrors "cosmossdk.io/errors"

// x/service module sentinel errors
var (
ErrServiceInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message")
ErrServiceDuplicateIndex = sdkerrors.Register(ModuleName, 1101, "duplicate index when adding a new service")
ErrServiceInvalidAddress = sdkerrors.Register(ModuleName, 1102, "invalid address when adding a new service")
ErrServiceMissingID = sdkerrors.Register(ModuleName, 1103, "missing service ID")
ErrServiceMissingName = sdkerrors.Register(ModuleName, 1104, "missing service name")
ErrServiceAlreadyExists = sdkerrors.Register(ModuleName, 1105, "service already exists")
ErrServiceInvalidServiceFee = sdkerrors.Register(ModuleName, 1106, "invalid ServiceFee")
ErrServiceAccountNotFound = sdkerrors.Register(ModuleName, 1107, "account not found")
ErrServiceNotEnoughFunds = sdkerrors.Register(ModuleName, 1108, "not enough funds to add service")
ErrServiceFailedToDeductFee = sdkerrors.Register(ModuleName, 1109, "failed to deduct fee")
ErrServiceInvalidRelayResponse = sdkerrors.Register(ModuleName, 1110, "invalid relay response")
ErrServiceInvalidRelayRequest = sdkerrors.Register(ModuleName, 1111, "invalid relay request")
ErrServiceInvalidSigner = sdkerrors.Register(ModuleName, 1100, "expected gov account as only signer for proposal message")
ErrServiceDuplicateIndex = sdkerrors.Register(ModuleName, 1101, "duplicate index when adding a new service")
ErrServiceInvalidAddress = sdkerrors.Register(ModuleName, 1102, "invalid address when adding a new service")
ErrServiceMissingID = sdkerrors.Register(ModuleName, 1103, "missing service ID")
ErrServiceMissingName = sdkerrors.Register(ModuleName, 1104, "missing service name")
ErrServiceAlreadyExists = sdkerrors.Register(ModuleName, 1105, "service already exists")
ErrServiceInvalidServiceFee = sdkerrors.Register(ModuleName, 1106, "invalid ServiceFee")
ErrServiceAccountNotFound = sdkerrors.Register(ModuleName, 1107, "account not found")
ErrServiceNotEnoughFunds = sdkerrors.Register(ModuleName, 1108, "not enough funds to add service")
ErrServiceFailedToDeductFee = sdkerrors.Register(ModuleName, 1109, "failed to deduct fee")
ErrServiceInvalidRelayResponse = sdkerrors.Register(ModuleName, 1110, "invalid relay response")
ErrServiceInvalidRelayRequest = sdkerrors.Register(ModuleName, 1111, "invalid relay request")
ErrServiceInvalidComputUnitsPerRelay = sdkerrors.Register(ModuleName, 1112, "invalid compute units per relay")
)
31 changes: 25 additions & 6 deletions x/service/types/message_add_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,25 @@ package types

import (
sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/pokt-network/poktroll/x/shared/types"
)

const (
DefaultComputeUnitsPerRelay uint64 = 1
// ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service
// ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service.
// TODO_MAINNET: The reason we have a maximum is to account for potential integer overflows. This is
// something that needs to be revisited or reconsidered prior to mainnet.

ComputeUnitsPerRelayMax uint64 = 2 ^ 16
)

var _ sdk.Msg = (*MsgAddService)(nil)

func NewMsgAddService(address, serviceId, serviceName string) *MsgAddService {
func NewMsgAddService(address, serviceId, serviceName string, computeUnitsPerRelay uint64) *MsgAddService {
return &MsgAddService{
Address: address,
Service: types.Service{
Id: serviceId,
Name: serviceName,
},
Service: *types.NewService(
serviceId,
serviceName,
computeUnitsPerRelay,
),
}
}

Expand All @@ -30,5 +36,18 @@ func (msg *MsgAddService) ValidateBasic() error {
if msg.Service.Name == "" {
return ErrServiceMissingName
}
if err := ValidateComputeUnitsPerRelay(msg.Service.ComputeUnitsPerRelay); err != nil {
return err
}
return nil
}

// ValidateComputeUnitsPerRelay makes sure the compute units per relay is a valid value
func ValidateComputeUnitsPerRelay(computeUnitsPerRelay uint64) error {
if computeUnitsPerRelay == 0 {
Olshansk marked this conversation as resolved.
Show resolved Hide resolved
return ErrServiceInvalidComputUnitsPerRelay.Wrap("compute units per relay must be greater than 0")
} else if computeUnitsPerRelay > ComputeUnitsPerRelayMax {
return ErrServiceInvalidComputUnitsPerRelay.Wrapf("compute units per relay must be less than %d", ComputeUnitsPerRelayMax)
}
Comment on lines +49 to +53
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the computation of ComputeUnitsPerRelayMax.

The current computation of ComputeUnitsPerRelayMax uses the bitwise XOR operator instead of the power operator.

- ComputeUnitsPerRelayMax uint64 = 2 ^ 16
+ ComputeUnitsPerRelayMax uint64 = 1 << 16
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if computeUnitsPerRelay == 0 {
return ErrServiceInvalidComputUnitsPerRelay.Wrap("compute units per relay must be greater than 0")
} else if computeUnitsPerRelay > ComputeUnitsPerRelayMax {
return ErrServiceInvalidComputUnitsPerRelay.Wrapf("compute units per relay must be less than %d", ComputeUnitsPerRelayMax)
}
ComputeUnitsPerRelayMax uint64 = 1 << 16

return nil
}
45 changes: 44 additions & 1 deletion x/service/types/message_add_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,18 @@ func TestMsgAddService_ValidateBasic(t *testing.T) {
Service: sharedtypes.Service{Id: "svc1"}, // Name intentionally omitted
},
expectedErr: ErrServiceMissingName,
}, {
desc: "valid service supplier address - zero compute units per relay",
msg: MsgAddService{
Comment on lines +39 to +41
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test case for negative compute units per relay.

Consider adding a test case to validate that negative values for ComputeUnitsPerRelay are handled correctly, even though uint64 should not allow negative values.

{
  desc: "valid service supplier address - negative compute units per relay",
  msg: MsgAddService{
    Address: sample.AccAddress(),
    Service: sharedtypes.Service{Id: "svc1", Name: "service name", ComputeUnitsPerRelay: ^uint64(0)},
  },
  expectedErr: ErrServiceInvalidComputUnitsPerRelay,
}

Address: sample.AccAddress(),
Service: sharedtypes.Service{Id: "svc1", Name: "service name", ComputeUnitsPerRelay: 0},
},
expectedErr: ErrServiceInvalidComputUnitsPerRelay,
}, {
desc: "valid service supplier address and service",
msg: MsgAddService{
Address: sample.AccAddress(),
Service: sharedtypes.Service{Id: "svc1", Name: "service name"},
Service: sharedtypes.Service{Id: "svc1", Name: "service name", ComputeUnitsPerRelay: 1},
},
expectedErr: nil,
},
Expand All @@ -56,3 +63,39 @@ func TestMsgAddService_ValidateBasic(t *testing.T) {
})
}
}

func TestValidateComputeUnitsPerRelay(t *testing.T) {
tests := []struct {
desc string
computeUnitsPerRelay uint64
expectedErr error
}{
{
desc: "zero compute units per relay",
computeUnitsPerRelay: 0,
expectedErr: ErrServiceInvalidComputUnitsPerRelay,
}, {
desc: "valid compute units per relay",
computeUnitsPerRelay: 1,
expectedErr: nil,
}, {
desc: "max compute units per relay",
computeUnitsPerRelay: ComputeUnitsPerRelayMax,
expectedErr: nil,
}, {
desc: "compute units per relay greater than max",
computeUnitsPerRelay: ComputeUnitsPerRelayMax + 1,
expectedErr: ErrServiceInvalidComputUnitsPerRelay,
},
}
for _, test := range tests {
t.Run(test.desc, func(t *testing.T) {
err := ValidateComputeUnitsPerRelay(test.computeUnitsPerRelay)
if test.expectedErr != nil {
require.ErrorIs(t, err, test.expectedErr)
return
}
require.NoError(t, err)
})
}
}
10 changes: 10 additions & 0 deletions x/shared/types/service.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package types

// NewService creates a new Service instance
Olshansk marked this conversation as resolved.
Show resolved Hide resolved
func NewService(id string, name string, computeUnitsPerRelay uint64) *Service {
return &Service{
Id: id,
Name: name,
ComputeUnitsPerRelay: computeUnitsPerRelay,
}
}
Loading