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

Passing result to subsequent steps, implement JavaScript runner #70

Merged
merged 21 commits into from
Dec 30, 2024

Conversation

v9n
Copy link
Member

@v9n v9n commented Dec 12, 2024

This PR implements:

  • Data from previous step will be feeded to next step using the name as variable. if name has space or invalid character, it will be replace with _
  • Switch expr -> javascript. no more expr
  • Implement javascript runner. We cannot use module yet but all primitive js is workin. js is es6 so we can use async/await and arrow function.

@v9n v9n changed the base branch from main to rename-things December 12, 2024 17:18
@v9n v9n marked this pull request as draft December 12, 2024 18:54
@v9n v9n force-pushed the passing-result-to-subsequent-steps branch from aa80fe1 to 0a2ab8a Compare December 27, 2024 19:28
@v9n v9n changed the base branch from rename-things to multiple-fix-and-improvement December 27, 2024 19:28
@v9n v9n changed the title Passing result to subsequent steps Passing result to subsequent steps, implement JavaScript runner Dec 27, 2024
@v9n v9n marked this pull request as ready for review December 27, 2024 20:17
@v9n v9n force-pushed the passing-result-to-subsequent-steps branch from fcaaee2 to ef7bf2c Compare December 27, 2024 20:22
@v9n v9n self-assigned this Dec 27, 2024
method: "POST",
body: `JSON.stringify({
chat_id:-4609037622,
text: \`Node IP is: \${getIpAddress.data.ip}.\nCurrent USDC liquidity rate in RAY unit is \${getReserveUSDC.data.reserves[0].liquidityRate} \`
Copy link
Member Author

Choose a reason for hiding this comment

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

@chrisli30 This shows how to access previous step otput. Notice getIpAddress is the name of a step. That name will be standarized to a var. if the name is get ip address we will convert to get_ip_address any non valid char is converted to _.

getIpAddress is result of a rest call
getReserveUSDC is result of a graphql call

Copy link
Member

Choose a reason for hiding this comment

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

okay, all node Ids from studio are Ulid, so we just need to make sure all Ulid could be valid variable names, right?

User Input like Get IP Address can only be node names, and is it true that we don’t use node.name for reference?

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, all node Ids from studio are Ulid, so we just need to make sure all Ulid could be valid variable names, right?

nah not just that. it's hard to find out(not just type out) what variable is what with ulid when you type it out in later step.

User Input like Get IP Address can only be node names, and is it true that we don’t use node.name for reference?

it's true.

We can also force the name to be a valid variable identifier like retool. It's auto convert space to _ (where i got this inspiration) and it prohibit the first char to not be a number

Copy link
Member

@chrisli30 chrisli30 left a comment

Choose a reason for hiding this comment

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

Approved! but I have left a few comments for review.

@@ -122,8 +123,8 @@ func NewAggregator(c *config.Config) (*Aggregator, error) {
clients, err := clients.BuildAll(chainioConfig, c.EcdsaPrivateKey, c.Logger)
if err != nil {
c.Logger.Errorf("Cannot create sdk clients", "err", err)
panic(err)
//return nil, err
// EigenLayer has update the contract and we cannot fetch the slasher anymore, we should upgrade the EigenSDK, right now we don't use it so it's ok to ignore this error
Copy link
Member

Choose a reason for hiding this comment

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

okay, this was the culprit of the slashing error, makes sense. 👌

Copy link
Member

Choose a reason for hiding this comment

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

I’d add a // TODO: to the beginning, so we can search all TODOs in codebase later.

}
}

// Temp disable until we figured out the event loop
Copy link
Member

Choose a reason for hiding this comment

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

Add a prefix // TODO: ?

method: "POST",
body: `JSON.stringify({
chat_id:-4609037622,
text: \`Node IP is: \${getIpAddress.data.ip}.\nCurrent USDC liquidity rate in RAY unit is \${getReserveUSDC.data.reserves[0].liquidityRate} \`
Copy link
Member

Choose a reason for hiding this comment

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

okay, all node Ids from studio are Ulid, so we just need to make sure all Ulid could be valid variable names, right?

User Input like Get IP Address can only be node names, and is it true that we don’t use node.name for reference?

@@ -297,7 +297,8 @@ func NewOperatorFromConfig(c OperatorConfig) (*Operator, error) {
)
if err != nil {
logger.Error("Cannot create AvsWriter", "err", err)
return nil, err
// EigenLayer has update the contract and we cannot fetch the slasher anymore, we should upgrade the EigenSDK, right now we don't use it so it's ok to ignore this error
Copy link
Member

Choose a reason for hiding this comment

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

Add prefix // TODO: so it’s easy to find all pending code.

Copy link
Member

@chrisli30 chrisli30 left a comment

Choose a reason for hiding this comment

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

Just saw the description "Data from previous step will be feeded to next step using the name as variable. if name has space or invalid character, it will be replace with _"

I’d avoid manipulating the id of the node cause that’s error-prone. There’s no reason that a node id is not fixed nor deterministic. Could we use ulid as the id of the node, for referencing previous nodes, to avoid and string modification?

We are good to merge this PR and iterate on this previous step identifier function.

@v9n
Copy link
Member Author

v9n commented Dec 30, 2024

@chrisli30

Just saw the description "Data from previous step will be feeded to next step using the name as variable. if name has space or invalid character, it will be replace with _"

I’d avoid manipulating the id of the node cause that’s error-prone. There’s no reason that a node id is not fixed nor deterministic.

No, you mis-understood it. We don't manipulating the id or anyhing at all.

This is how you refer to it as the variable.

Could we use ulid as the id of the node, for referencing previous nodes, to avoid and string modification?
no, we couldnot use ulid or uuid. this is the variable name. it cannot start with number for example

It's this

Imagine you had this

[
{ id: '12345abcdef',
  name: 'get price",}
{ id: 78910',
  javascriptcode: ' if (get price.data.value > 0) { a = 2 }'
}
]

let me know how you want to handle that case. we cannot use uuild or ulid because it may start with number .

on top of that, it's really hard to keep track of a variable if you have to use ulid/uuid as variable name. examplle, user has to write this code:

[
{ id: '12345abcdef',
  name: 'get price",}
{ id: 78910',
  javascriptcode: ' if (12345abcdef.data.value > 0) { a = 2 }'
}
]

Take a look at my example https://github.com/AvaProtocol/EigenLayer-AVS/pull/70/files#diff-a0e0283e24c8bc8fe874fde781384a5ce414c126f85aa7e2453b60ec44931388R730-R746 what would you expect an end user use as variable name for the previous step?

@v9n
Copy link
Member Author

v9n commented Dec 30, 2024

@chrisli30 please check retool as well. they auto convert the space to _ when you type on front-end. and they disallow number as first character.

disallow starting with number

CleanShot 2024-12-29 at 16 29 19@2x

force using name as a valid identifier name

CleanShot.2024-12-29.at.16.31.21.mp4

so on the front-end studio you can do the same to force that.

Base automatically changed from multiple-fix-and-improvement to main December 30, 2024 12:55
@v9n v9n merged commit 3b8201c into main Dec 30, 2024
4 checks passed
@v9n v9n deleted the passing-result-to-subsequent-steps branch December 30, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants