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

Calculate max allowance #45

Open
wants to merge 32 commits into
base: primary
Choose a base branch
from
Open

Calculate max allowance #45

wants to merge 32 commits into from

Conversation

navFooh
Copy link
Contributor

@navFooh navFooh commented Mar 24, 2022

This is a first pass on the max allowance caluculation. I had to make a few assumptions along the way which I'll explain below, let me know what you think. Please also read the comments in getMaxAllowance.go.

  • The user object is set up to support multiple accounts. The User.HasAccountOlderThan() function tests whether either one of the accounts meets the required age. However, the User.GetMaxAllowance() function requires a GitHub account and returns an error when this is not found. Currently I understand we only support GitHub accounts, but if another account type meets the age check, an error can still occur in User.GetMaxAllowance(). Is this ok for now or should we return the base allowance without an error for non-GitHub users?

  • As mentioned on Discord, GitHub currently returns a maximum account history of 300 events. This causes a problem when there is too much recent activity, because the history won't go back to the required 3 months. When this occurs, I currently give the user the benefit of the doubt and pass the check anyway. This does require the "maximum event count" which is returned from github to be hardcoded. It also means that you can pass the test by generating 300 events in a single day of the current month.

  • Instead of generating a "score" to transform into the "max allowance bytes", I let the method return the max allowance straight away. It seemed more logical to do it this way, let me know what you think.

  • I wasn't sure whether the following check should use the baseAllowance or the calculated maxAllowance. I assumed the latter:

fiftyDataCaps := types.BigMul(maxAllowance, types.NewInt(50))
if dataCap.LessThanEqual(fiftyDataCaps) {
	slackNotification := "LOW DATA CAP: " + dataCap.String()
	sendSlackNotification("https://errors.glif.io/verifier-low-data-cap", slackNotification)
}
  • I added two routes for testing, one that uses the JWT of the signed in user (/max-allowance/:target_addr) and another where you can supply your github user name (/max-allowance-github/:github_user/:target_addr).

  • I've implemented the following slack hook for when the max allowance calculation fails, this would need to be implemented on Slack as well:

sendSlackNotification("https://errors.glif.io/max-allowance-failed", slackNotification)
  • I've renamed the MaxAllowanceBytes / MAX_ALLOWANCE_BYTES environment variable to BaseAllowanceBytes / BASE_ALLOWANCE_BYTES, in order to indicate better what it is used for at the moment

@navFooh navFooh requested a review from Schwartz10 March 24, 2022 14:51
@navFooh
Copy link
Contributor Author

navFooh commented Mar 24, 2022

Another thing, I currently put the Fil+ API key in the environment variables, but just noticed this endpoint:
https://documenter.getpostman.com/view/131998/Tzsim4NU#f10e7466-8752-49c1-aeb9-b3d5387d8a70

Should we make the server call that endpoint on startup instead?

Copy link
Contributor

@Schwartz10 Schwartz10 left a comment

Choose a reason for hiding this comment

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

Great work on this @navFooh !!! Impressed at how fast you got up to speed on go. Responding to your bullets in order:

  • It is OK to just assume we only support GH for now.
  • This seems ok to me for now.. I dont see a huge incentive to bot this system for the amount of data cap we're giving. Just curious, did the same thing happen with the access token too?
  • Agree on the decision here.
  • Responded with a comment to this one - we can just hardcode the amount.
  • Routes look good to me
  • I would just delete this slack hook for now and lets try to figure out a better way for notifications via sentry - does that make sense to you?
  • The rename looks good

server.go Outdated
@@ -373,7 +434,7 @@ func serveCheckAccountRemainingBytes(c *gin.Context) {
}

type Response struct {
RemainingBytes string `json:"remainingBytes"`
RemainingBytes string `json:"remainingBytes"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a different go formatter? Or was this just wrongly formatted in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering the same as well. I'm using VS Code and installed the official Go extension from Google. This prompted to install some additional Go modules for formatting which I did as well.

I can also not modify the env.go file for example in VS Code without reformatting most of the continuation indent, so I used Notepad to add a few lines there 😅

Not sure if I should change my formatter settings?

* rel = `test`
* returns `https://example.com`
*/
func getLinkHeaderURI(link string, rel string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

so this basically just formats the URL so we can paginate through results properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whenever you make a paginated request to the API, it returns a link header that looks as follows:

link: <https://api.github.com/user/394452/events?per_page=100&page=1>; rel="prev", <https://api.github.com/user/394452/events?per_page=100&page=3>; rel="next", <https://api.github.com/user/394452/events?per_page=100&page=3>; rel="last", <https://api.github.com/user/394452/events?per_page=100&page=1>; rel="first"

Baiscally this is one big string, so getting the URL that belongs to rel="next" requires something like a parser or regex, which I wrote for this. It's very convenient to use this header, since you'll know you've reached the last page when rel="next" is not present anymore (getLinkHeaderURI() returns an empty string)

server.go Outdated Show resolved Hide resolved
@navFooh
Copy link
Contributor Author

navFooh commented Mar 25, 2022

  • This seems ok to me for now.. I dont see a huge incentive to bot this system for the amount of data cap we're giving. Just curious, did the same thing happen with the access token too?

I tried and it doesn't make a difference with the access token, there is still a limit, read more here.

One thing that does change with Access tokens is that the rate limit is bumped from 60 requests per hour to 5000 requests per hour. This might be important because a single user can use 3 requests with the pagination, so a possible limit of 20 per hour. Should we make this a GitHub Application?

  • I would just delete this slack hook for now and lets try to figure out a better way for notifications via sentry - does that make sense to you?

Yes that makes sense! Let's do Sentry

* Add go-logger package

* Get SentryDsn and SentryEnv from environment variables

* Tidied go modules

* Implement go-logger package

* Update go-logger

* Add Sentry log level

* Update go-logger

* Change slack notifications for logger

* Update error logging

* Remove last of slack notifications

* Update info logging

* Update Gin error handling logger

* Remove fmt for logger

* Update log in lotus.go

* Update oauth logs

* Use JSON marshall / unmarshall for GitHub OAuth request

* Use Panic from logger package
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