-
Notifications
You must be signed in to change notification settings - Fork 12
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
Changes from 3 commits
f1194ec
4491832
37555c7
3b32119
826404f
dabe8f1
dcb6fbe
5e9c3b8
cd2fef0
517ad39
753f79a
0d5dbce
0228131
717843d
f7559ca
1305ed6
a227d74
af04fe2
0a0d786
12ea443
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 with compute_units_to_tokens_multipler to calculate the cost of a relay | ||||||
uint64 compute_units_per_relay = 3; // Compute units required per relay | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
// ApplicationServiceConfig holds the service configuration the application stakes for | ||||||
|
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" | ||||||
|
@@ -15,18 +16,27 @@ 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), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to show that it's optional
Suggested change
|
||||||
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)`, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] | ||||||
|
||||||
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]) | ||||||
} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a log statement in the else case saying: "Use default ..." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Olshansk from what I see, the loggers are tied to the keepers. In this context, I don't know how to access the appropriate logger. I looked in all of the other CLI handlers and I do not see any previous examples of printing logs from this context. Do you just want a simple |
||||||
|
||||||
clientCtx, err := client.GetClientTxContext(cmd) | ||||||
if err != nil { | ||||||
return err | ||||||
|
@@ -36,6 +46,7 @@ $ poktrolld tx service add-service "svc1" "service_one" --keyring-backend test - | |||||
clientCtx.GetFromAddress().String(), | ||||||
serviceIdStr, | ||||||
serviceNameStr, | ||||||
computeUnitsPerRelay, | ||||||
) | ||||||
if err := msg.ValidateBasic(); err != nil { | ||||||
return err | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ package service_test | |
|
||
import ( | ||
"fmt" | ||
"strconv" | ||
"testing" | ||
|
||
sdkerrors "cosmossdk.io/errors" | ||
|
@@ -50,14 +51,16 @@ func TestCLI_AddService(t *testing.T) { | |
// Wait for a new block to be committed | ||
require.NoError(t, net.WaitForNextBlock()) | ||
|
||
// Prepare two valid services | ||
// Prepare three valid services | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where's the third one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, I'll remove that. |
||
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{ | ||
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we make this invalid? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
|
@@ -113,11 +125,22 @@ func TestCLI_AddService(t *testing.T) { | |
// Wait for a new block to be committed | ||
require.NoError(t, net.WaitForNextBlock()) | ||
|
||
var args []string | ||
// Prepare the arguments for the CLI command | ||
args := []string{ | ||
test.service.Id, | ||
test.service.Name, | ||
fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress), | ||
// Only include compute units per relay argument if provided | ||
if test.service.ComputeUnitsPerRelay > 0 { | ||
args = []string{ | ||
test.service.Id, | ||
test.service.Name, | ||
strconv.FormatUint(test.service.ComputeUnitsPerRelay, 10), | ||
fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress), | ||
} | ||
} else { | ||
args = []string{ | ||
test.service.Id, | ||
test.service.Name, | ||
fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress), | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wdyt of this? argsAndFlags := []string{
test.service.Id,
test.service.Name,
}
if test.service.ComputeUnitsPerRelay > 0 {
argsAndFlags := append(args, strconv.FormatUint(test.service.ComputeUnitsPerRelay, 10),)
}
argsAndFlags = append(args, fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress)) |
||
} | ||
args = append(args, commonArgs...) | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 - no compute units per relay", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
msg: MsgAddService{ | ||||||
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, | ||||||
}, | ||||||
|
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, | ||
} | ||
} |
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.