Std #1
Open
Std #1
+6,284
−3,316
Conversation
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
…vider is participating in.
Script that lists all the active rituals that a specified staking provider is participating in
Co-authored-by: David Núñez <[email protected]>
Coordinator: moves rituals from array to mapping
…es to ape 0.8.8 but not to ape 0.7.x which we currently use.
Undo previous change regarding how receipts were obtained
Add duration calculation for subscription based rituals
…ifferent than /bucket_sampling used on mainnet.
Fix use of Porter for sampling
Don't check for ape-infura when not using infura
…ct deployment. Co-authored-by: James Campbell <[email protected]>
Upgrade tapir Coordinator
Relock dependencies to use `ape-etherscan` >= 0.8.3
…st-deployment. The contracts can either be obtained from the common domain registry, or a customized registry path. Ensure that implementation contract for proxies are the contracts being verified.
…script Working contract verification script to verify individual contracts post-deployment
Add function to withdraw all tokens from Coordinator for a token address
Move some common test definitions to conftest.py
Make Coordinator aware of transcript size
ReimbursementPool on Polygon Mainnet
Update Coordinator ABI on mainnet registry
|
Your free trial has expired. To keep using Ellipsis, sign up at https://app.ellipsis.dev or contact us. |
Reviewer's Guide by SourceryThis pull request introduces several changes to the Coordinator contract, primarily to support upgrades and improve functionality. It also updates the requirements for several dependencies. Class diagram showing changes to Coordinator contractclassDiagram
class Coordinator {
+mapping(uint256 => Ritual) _rituals
+uint256 numberOfRituals
-mapping(address => ParticipantKey[]) participantKeysHistory
-mapping(bytes32 => uint32) ritualPublicKeyRegistry
-mapping(IFeeModel => bool) feeModelsRegistry
+initializeNumberOfRituals()
+rituals(uint256 ritualId)
-storageRitual(uint32 ritualId)
+withdrawAllTokens(IERC20 token)
}
note for Coordinator "Changed storage layout:
- Removed rituals array
- Added _rituals mapping
- Added numberOfRituals counter"
Class diagram showing new ReimbursementPool contractclassDiagram
class ReimbursementPool {
+mapping(address => bool) isAuthorized
+uint256 staticGas
+uint256 maxGasPrice
+constructor(uint256 _staticGas, uint256 _maxGasPrice)
+refund(uint256 gasSpent, address receiver)
+authorize(address _contract)
+unauthorize(address _contract)
+setStaticGas(uint256 _staticGas)
+setMaxGasPrice(uint256 _maxGasPrice)
+withdrawAll(address receiver)
+withdraw(uint256 amount, address receiver)
}
ReimbursementPool --|> Ownable
ReimbursementPool --|> ReentrancyGuard
ReimbursementPool --|> IReimbursementPool
Class diagram showing BqETHSubscription renamed to StandardSubscriptionclassDiagram
class StandardSubscription {
+struct Billing
}
StandardSubscription --|> EncryptorSlotsSubscription
StandardSubscription --|> Initializable
StandardSubscription --|> OwnableUpgradeable
note for StandardSubscription "Renamed from BqETHSubscription
No functional changes"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @theref - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding documentation about the upgrade process and reimbursement pool configuration to help users understand these significant new features
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -105,30 +106,55 @@ def cli( | |||
| message="Cannot specify --min-version when using --handpicked.", | |||
| ) | |||
|
|
|||
| # Get the contracts from the registry | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the subscription duration calculation logic into a helper function to improve readability and maintainability.
The subscription duration calculation logic adds unnecessary complexity to the CLI script. Extract this into a helper function to improve readability:
def get_ritual_duration(fee_model: str, fee_model_contract, duration: int | None) -> int:
if fee_model != "BqETHSubscription":
if not duration:
raise click.BadOptionUsage("--duration", "Duration is required for non-subscription rituals")
return duration
total_duration = (
fee_model_contract.subscriptionPeriodDuration()
+ fee_model_contract.yellowPeriodDuration()
+ fee_model_contract.redPeriodDuration()
)
start = fee_model_contract.startOfSubscription()
if start == 0:
return total_duration
end = fee_model_contract.getEndOfSubscription()
now = chain.blocks.head.timestamp
if now > end:
raise ValueError("Subscription has already ended.")
return total_duration - (now - start + 100)This improves the main flow by:
- Separating subscription logic from ritual initiation
- Reducing nesting depth
- Making the duration calculation more explicit
- Handling validation consistently
| num_rituals = coordinator.numberOfRituals() | ||
|
|
||
| ritual_memberships = [] | ||
| for ritual_id in range(0, num_rituals): |
There was a problem hiding this comment.
suggestion (code-quality): Replace range(0, x) with range(x) (remove-zero-from-range)
Suggested change
| for ritual_id in range(0, num_rituals): | |
| for ritual_id in range(num_rituals): |
Explanation
The default starting value for a call torange() is 0, so it is unnecessary toexplicitly define it. This refactoring removes such zeros, slightly shortening
the code.
| from eth_account import Account | ||
| from hexbytes import HexBytes | ||
| from web3 import Web3 | ||
|
|
||
| from tests.conftest import ONE_DAY, gen_public_key, generate_transcript, RitualState |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
| @@ -6,41 +6,13 @@ | |||
| from eth_account.messages import encode_defunct | |||
| from web3 import Web3 | |||
|
|
|||
| from tests.conftest import gen_public_key, generate_transcript | |||
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
Comment on lines
+513
to
+519
| result = method( | ||
| *args, | ||
| sender=self._account, | ||
| # FIXME: Manual gas fees - #199 | ||
| # max_priority_fee="3 gwei", | ||
| # max_fee="120 gwei" | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Comment on lines
+657
to
658
| wrapped_instance = getattr(project, implementation.contract_type.name).at(proxy_address) | ||
| return wrapped_instance |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| wrapped_instance = getattr(project, implementation.contract_type.name).at(proxy_address) | |
| return wrapped_instance | |
| return getattr(project, implementation.contract_type.name).at(proxy_address) |
Comment on lines
+206
to
207
| result = sorted(ursulas, key=lambda x: x.lower()) | ||
| return result |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| result = sorted(ursulas, key=lambda x: x.lower()) | |
| return result | |
| return sorted(ursulas, key=lambda x: x.lower()) |
| ) | ||
| verify_contracts(list(contracts.values())) | ||
| def cli(network, domain, contract_name, registry_filepath): | ||
| """Verify a deployed contract.""" |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Explicitly raise from a previous error (
raise-from-previous-error)
Comment on lines
+75
to
76
| proxy_contract = project.ExtendedCoordinator.at(proxy.address) | ||
| return proxy_contract |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| proxy_contract = project.ExtendedCoordinator.at(proxy.address) | |
| return proxy_contract | |
| return project.ExtendedCoordinator.at(proxy.address) |
Comment on lines
+104
to
+105
| proxy_contract = project.GlobalAllowList.at(proxy.address) | ||
| return proxy_contract |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| proxy_contract = project.GlobalAllowList.at(proxy.address) | |
| return proxy_contract | |
| return project.GlobalAllowList.at(proxy.address) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers:
Summary by Sourcery
Upgrade contracts to use a new
Ritualstruct, and add a treasury role to theCoordinatorcontract. Migrate existing rituals to the new struct.